ESQL: Support filters on inlinestats #132934

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


    /**
     * Replaces the stubbed source with the actual source.
     * 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"

	* 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

ESQL: Support filters on inlinestats #132934

ESQL: Support filters on inlinestats #132934

Uh oh!

Conversation

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

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 19, 2025

Uh oh!

elasticsearchmachine commented Aug 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

astefan commented Aug 14, 2025 •

edited

Loading

bpintea left a comment •

edited

Loading

bpintea left a comment •

edited

Loading