ESQL: Support filters on inlinestats by astefan · Pull Request #132934 · elastic/elasticsearch

astefan · 2025-08-14T15:00:01Z

Addresses #124741

…filters_on_inlinestats

elasticsearchmachine · 2025-08-19T07:06:27Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2025-08-19T07:06:28Z

Hi @astefan, I've created a changelog YAML for you.

astefan · 2025-08-19T07:06:58Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

+10003          |Parto          |null           |0              |null           |M              |1               |4             
+;
+
+prunedInlinestatsFollowedByInlinestats_GroupByOneFieldOnSecondInlinestats-Ignore


This will be as a follow up investigation. I did try to fix it several times, ran out of good ideas.

astefan · 2025-08-19T07:07:29Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

+null             |[0, 0, 0, 0]   |[Berni, Chirstian, Amabile, Berni, Bojan, Chirstian, Anneke, Chirstian]|[Head Human Resources, Reporting Analyst, Support Engineer, Tech Lead]|10004          
+;
+
+prunedInlinestatsFollowedByinlinestats-Ignore


This will be as a follow up investigation. I did try to fix it several times, ran out of good ideas. This is in the same ballpark failure set as the other one.

astefan · 2025-08-19T07:16:35Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

 emp_no:integer
 5
 ;
+///////////////////////////


Almost all the tests below have been ported from the same set of stats tests with filters and adapted to inlinestats.

astefan · 2025-08-19T07:17:39Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+    protected LogicalPlan rule(LogicalPlan plan) {
+        Aggregate aggregate;
+        Holder<InlineJoin> ij = new Holder<>();
+        if (plan instanceof Aggregate a) {


This code block, in a nutshell, finds out if the Aggregate comes from inlinejoin or is a standalone one.

astefan · 2025-08-19T07:18:54Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+            return plan; // not an Aggregate or InlineJoin, nothing to do
+        }
+
+        if (aggregate != null) {


The code here is the same as before, but now wrapped in this condition related to aggregate being null or not.

astefan · 2025-08-19T07:19:31Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

-                plan = new Project(aggregate.source(), plan, newProjections);
+            if (newEvals.isEmpty() == false) {
+                if (newAggs.isEmpty()) { // the Aggregate node is pruned
+                    if (ij.get() != null) { // this is an Aggregate part of right-hand side of an InlineJoin


The meat of the PR. Basically, a new "branch" in dealing with pruning Aggs in InlineJoin specifically.

astefan · 2025-08-19T07:19:43Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                        // project the correct output (the one of the former inlinejoin) and remove the InlineJoin altogether,
+                        // replacing it with its right-hand side followed by its left-hand side
+                        plan = new Project(newIJ.source(), newIJ.right(), newIJ.output());
+                    } else { // this is a standalone Aggregate


Same code as before.

astefan · 2025-08-19T07:19:52Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                            return newPlan;
+                        });
+                    } else { // this is a standalone Aggregate
+                        plan = aggregate.with(aggregate.child(), aggregate.groupings(), newAggs);


Same code as before.

astefan · 2025-08-19T07:20:08Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                        plan = localRelation(aggregate.source(), newEvals);
+                    }
+                } else {
+                    if (ij.get() != null) { // this is an Aggregate part of right-hand side of an InlineJoin


The meat of the PR (specific handling for InlineJoin).

astefan · 2025-08-19T07:20:42Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

        assertThat(Expressions.names(agg.aggregates()), contains("sum(salary)", "sum(salary) WheRe last_name ==   \"Doe\""));
    }

-    /**


Moved these to a new class, specific to ReplaceStatsFilteredAggWithEval rule.

…lasticsearch into filters_on_inlinestats

bpintea

Left mostly minor notes, except for one.

bpintea · 2025-08-19T20:08:33Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

-        List<NamedExpression> newProjections = new ArrayList<>(oldAggSize);
+    protected LogicalPlan rule(LogicalPlan plan) {
+        Aggregate aggregate;
+        Holder<InlineJoin> ij = new Holder<>();


I think you can use a simple variable here, you don't need a holder. IIUC, there's a single place where the var would be used in a lambda, you may assign it locally in the respective branch (or right after this detection block) to make it effectively final.

bpintea · 2025-08-19T20:10:23Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+        Holder<InlineJoin> ij = new Holder<>();
+        if (plan instanceof Aggregate a) {
+            aggregate = a;
+        } else if (plan instanceof InlineJoin) {


Suggested change

} else if (plan instanceof InlineJoin) {

} else if (plan instanceof InlineJoin inlineJoin) {

to no longer need to cast.

bpintea · 2025-08-19T20:16:32Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+            ij.get().right().forEachDown(p -> {
+                if (aggHolder.get() == null && p instanceof Aggregate a) {
+                    aggHolder.set(a);
+                }
+            });


Suggested change

ij.get().right().forEachDown(p -> {

if (aggHolder.get() == null && p instanceof Aggregate a) {

aggHolder.set(a);

}

});

inlineJoin.right().forEachDown(Aggregate.class, aggHolder::setIfAbsent);

bpintea · 2025-08-19T20:27:24Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                if (newAggs.isEmpty()) { // the Aggregate node is pruned
+                    if (ij.get() != null) { // this is an Aggregate part of right-hand side of an InlineJoin
+                        // replace the right hand side Aggregate with an Eval
+                        InlineJoin newIJ = new InlineJoin(ij.get().source(), ij.get().left(), ij.get().right().transformDown(p -> {


Suggested change

InlineJoin newIJ = new InlineJoin(ij.get().source(), ij.get().left(), ij.get().right().transformDown(p -> {

InlineJoin newIJ = inlineJoin.replaceRight(inlineJoin.right().transformDown(p -> {

with inlineJoin the effectively final var.

bpintea · 2025-08-19T20:40:45Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                } else {
+                    if (ij.get() != null) { // this is an Aggregate part of right-hand side of an InlineJoin
+                        // only update the Aggregate and add an Eval for the removed aggregations
+                        plan = ij.get().transformUp(Aggregate.class, agg -> {


Minor: I think this'll walk the entire (sub)tree of InlineJoin, while an update is really only possible on the right hand-side. Would it be worth applying the change there only?

bpintea · 2025-08-19T20:58:46Z

...g/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEvalTests.java

+            | inlinestats max = max(salary), max_a = max(salary) where null,
+                    min = min(salary), min_a = min(salary) where to_string(null) == "abc"


Aesthetic:

Suggested change

| inlinestats max = max(salary), max_a = max(salary) where null,

min = min(salary), min_a = min(salary) where to_string(null) == "abc"

| inlinestats max = max(salary),

max_a = max(salary) where null,

min = min(salary),

min_a = min(salary) where to_string(null) == "abc"

bpintea · 2025-08-19T21:13:15Z

...g/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEvalTests.java

+        as(limit.child(), EsRelation.class);
+    }
+
+    /**


Suggested change

/**

/*

to prevent having the block be interpreted as javadoc. Here and above, where there's no @code wrapping (which isn't ideal, as it's wrapped in IntelliJ; but that was just copied).

bpintea · 2025-08-19T21:17:07Z

...g/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEvalTests.java

It's great to start new files (started working on splitting the mighty x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java, but it'd be good to allocate a packet for it, since there'll be many "logical plan optimiser tests" (vs local ~ etc.).
But anyways, this can be added as is and I'll incorporate this later in that work.

Note though that this approach (separate package only for `local) is the same in the non-test package.

bpintea · 2025-08-19T21:48:17Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                        }), ij.get().config());
+                        // project the correct output (the one of the former inlinejoin) and remove the InlineJoin altogether,
+                        // replacing it with its right-hand side followed by its left-hand side
+                        plan = new Project(newIJ.source(), newIJ.right(), newIJ.output());


This Project is what's basically returned. Can't the entire InlineJoin transformation be skipped and only transform with the right hand-side of the "old" InlineJoin?

bpintea · 2025-08-19T21:54:31Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                            } else if (p instanceof StubRelation) {
+                                // Remove the StubRelation since the right-hand side of the join is now part of the main plan
+                                // and it won't be executed separately by the EsqlSession inlinestats planning.
+                                p = ij.get().left();


Not quite positive, but this might be the issue for a stack overflow with:

from employees | inlinestats count = count(*) where false | inlinestats cc = count_distinct(emp_no) where false

Nice catch @bpintea.
This raises an interesting discussion point: the fact that the right-hand side of a InlineJoin must contain a single StubRelation and that anything that has to do with StubRelations has to happen only once per InlineJoin right-hand side. I've made some changes to InlineJoin because of this.

…filters_on_inlinestats

bpintea

LGTM.
I'd update the PR description (and title?), as it's not really implementing the support, only optimising that.

bpintea · 2025-08-25T17:13:48Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java

+                    doneReplacing.set(true);
+                    return up.replaceChild(stubReplacement);
+                }
+                assert false : "Expected to replace a single StubRelation in the plan, but found more than one";


Better throw here:

this will bring down the whole node in CI (and fail many other unrelated tests).

it will do nothing in production: better fail the query.

bpintea · 2025-08-25T17:14:37Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java

@@ -80,17 +80,26 @@ public static LogicalPlan inlineData(InlineJoin target, LocalRelation data) {
     * NOTE: this will replace all {@link StubRelation}s found with the source and the method is meant to be used to replace one node only


Suggested change

* NOTE: this will replace all {@link StubRelation}s found with the source and the method is meant to be used to replace one node only

* NOTE: this will replace the first {@link StubRelation}s found with the source and the method is meant to be used to replace one node only

bpintea · 2025-08-25T17:18:15Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java

-        return stubbed.transformUp(UnaryPlan.class, up -> {
+        Holder<Boolean> doneReplacing = new Holder<>(false);
+        var result = stubbedPlan.transformUp(UnaryPlan.class, up -> {
            if (up.child() instanceof StubRelation) {


This is not a change strictly related to the PR, but this assumes there will never be an InlineJoin having a right child be a StubRelation. I'm not sure if this can occur now, but ideally we wouldn't rely on it, an intermediary state of the optimisations could theoretically produce it.
I.e. it's not an invariant.

bpintea · 2025-08-25T17:19:14Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/InlineJoin.java

            return up;
        });
+
+        assert doneReplacing.get() : "Expected to replace a single StubRelation in the plan, but none found";


Maybe same here, we could throw, instead of an assert?

bpintea · 2025-08-25T17:23:33Z

...va/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsFilteredAggWithEval.java

+                        var newRight = ij.right().transformDown(Aggregate.class, agg -> {
+                            LogicalPlan p = agg;
+                            if (agg == aggregate) {
+                                // the aggregate becomes a simple Eval since it's not needed anymore (it was replaced with Literals)
+                                p = new Eval(aggregate.source(), aggregate.child(), newEvals);
+                            }
+                            return p;
+                        });


Optional, but would be symmetrical to the other branch:

Suggested change

var newRight = ij.right().transformDown(Aggregate.class, agg -> {

LogicalPlan p = agg;

if (agg == aggregate) {

// the aggregate becomes a simple Eval since it's not needed anymore (it was replaced with Literals)

p = new Eval(aggregate.source(), aggregate.child(), newEvals);

}

return p;

});

// the aggregate becomes a simple Eval since it's not needed anymore (it was replaced with Literals)

var newRight = ij.right()

.transformDown(

Aggregate.class,

agg -> agg == aggregate ? new Eval(aggregate.source(), aggregate.child(), newEvals) : agg

);

bpintea · 2025-08-25T17:41:44Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

+10010          |null           |4                |6.949207097931448  |7.127229475750027|27921.220736207077|10             
+;
+
+twoConsecutiveInlinestatsWithFalseFiltes


I'd maybe add this part of the unit tests (ReplaceStatsFilteredAggWithEvalTests). It's interesting, as it produces this plan:

EsqlProject[[emp_no{f}#9, count{r}#5, cc{r}#8]] \_Eval[[0[LONG] AS count#5, 0[LONG] AS cc#8]] \_TopN[[Order[emp_no{f}#9,ASC,LAST]],3[INTEGER]] \_Limit[3[INTEGER],false] \_EsRelation[test][_meta_field{f}#15, emp_no{f}#9, first_name{f}#10, g..]

The TopN-Limit is probably avoidable, though it's not produced part of this change, it's the way we handle limit | sort | limit. But up to you.

…filters_on_inlinestats

astefan added 5 commits July 24, 2025 17:04

WIP

41a9297

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

3d728be

…filters_on_inlinestats

Update

b9cb4f3

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

5803c81

…filters_on_inlinestats

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

c565cf5

…filters_on_inlinestats

elasticsearchmachine added the v9.2.0 label Aug 14, 2025

Leftover

d852cda

astefan force-pushed the filters_on_inlinestats branch from 35b85ac to d852cda Compare August 14, 2025 15:03

astefan and others added 8 commits August 14, 2025 23:51

Update tests that need the new capability

16eb915

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

bef48d6

…filters_on_inlinestats

update

9b09bf2

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

5e45b6c

…filters_on_inlinestats

Revert change to PruneEmptyPlans.java

2332a4b

Unit tests

0ee6737

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

9b02911

…filters_on_inlinestats

Merge branch 'main' into filters_on_inlinestats

cb69c3e

astefan requested review from alex-spies and bpintea August 19, 2025 07:05

astefan marked this pull request as ready for review August 19, 2025 07:05

astefan added >enhancement :Analytics/ES|QL AKA ESQL labels Aug 19, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 19, 2025

Update docs/changelog/132934.yaml

0bd657d

astefan commented Aug 19, 2025

View reviewed changes

astefan added 2 commits August 19, 2025 11:17

More deterministic results by sorting on multi-values

3fdb65d

Merge branch 'filters_on_inlinestats' of https://github.com/astefan/e…

b6c38fd

…lasticsearch into filters_on_inlinestats

bpintea reviewed Aug 19, 2025

View reviewed changes

astefan and others added 7 commits August 21, 2025 07:41

Address reviews

8270a19

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

9c7bb68

…filters_on_inlinestats

Big fix for a StackoverflowError

f7ac82e

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

920a5ef

…filters_on_inlinestats

[CI] Auto commit changes from spotless

a82f067

Deterministic test

c5d8cef

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

b258234

…filters_on_inlinestats

bpintea approved these changes Aug 25, 2025

View reviewed changes

astefan added 2 commits August 26, 2025 09:35

Address reviews

8483389

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

1dceece

…filters_on_inlinestats

astefan merged commit b968ec9 into elastic:main Aug 26, 2025
33 checks passed

astefan deleted the filters_on_inlinestats branch August 26, 2025 07:33

	} else if (plan instanceof InlineJoin) {
	} else if (plan instanceof InlineJoin inlineJoin) {

	InlineJoin newIJ = new InlineJoin(ij.get().source(), ij.get().left(), ij.get().right().transformDown(p -> {
	InlineJoin newIJ = inlineJoin.replaceRight(inlineJoin.right().transformDown(p -> {

		\| inlinestats max = max(salary), max_a = max(salary) where null,
		min = min(salary), min_a = min(salary) where to_string(null) == "abc"

		@@ -80,17 +80,26 @@ public static LogicalPlan inlineData(InlineJoin target, LocalRelation data) {
		* NOTE: this will replace all {@link StubRelation}s found with the source and the method is meant to be used to replace one node only

	* NOTE: this will replace all {@link StubRelation}s found with the source and the method is meant to be used to replace one node only
	* NOTE: this will replace the first {@link StubRelation}s found with the source and the method is meant to be used to replace one node only

Conversation

astefan commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Aug 19, 2025

elasticsearchmachine commented Aug 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

3 participants

astefan commented Aug 14, 2025 •

edited

Loading

bpintea left a comment •

edited

Loading

bpintea left a comment •

edited

Loading