ESQL: remove incorrect inline stats pruning by astefan · Pull Request #141056 · elastic/elasticsearch

astefan · 2026-01-21T15:01:22Z

Removes incorrect pruning in inline stats that lacked taking into consideration the fact that groupings can be multi-valued.
Fixes #139359 (comment) (grouping by an mv fields can lead to inconsistencies in certain queries)
Fixes #139359
Fixes #140757

This also extracts a bunch of tests methods from the huge LogicalPlanOptimizerTests and moves them to a new test class - PruneColumnsTests which should include most of the tests that have in the spotlight the PruneColumns optimization rule.

…139359_fix_take2r

elasticsearchmachine · 2026-01-30T10:09:38Z

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

…139359_fix_take2

…search into 139359_fix_take2

elasticsearchmachine · 2026-01-30T10:34:40Z

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

astefan · 2026-01-30T10:40:56Z

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

+347.99              
+;
+
+inlineStatsGroupByNull_PruneDifferentInlineStats


This test covers both the bug fix here but also it's extended following #140027 merge

astefan · 2026-01-30T10:42:03Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumnsTests.java

+
+public class PruneColumnsTests extends AbstractLogicalPlanOptimizerTests {
+
+    public void testPruneUnusedEval() {


The start of this file contains already existent tests from LogicalPlanOptimizerTests. There is another comment down below with the new tests pertaining to this PR fix only.

astefan · 2026-01-30T10:43:24Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumnsTests.java

+        }
+    }
+
+    /*


Tests relevant for this PR's fix.

astefan · 2026-01-30T10:47:00Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumnsTests.java

+
+        // Left branch: Project with avg, decades, idecades
+        var leftProject = as(inlineJoin.left(), Project.class);
+        assertThat(Expressions.names(leftProject.projections()), is(List.of("avg", "decades")));


Here idecades has been projected away from the original version of this test method.

The comment could be updated too.

alex-spies

Thanks a lot @astefan !

As discussed before, I can't currently give this a deep review, but the approach looks good to me and I'm happy about the added spec tests :)

alex-spies · 2026-02-09T15:12:17Z

...in/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumns.java

        var right = pruneColumns(ij.right(), used, true);
-        if (right.output().isEmpty() || isLocalEmptyRelation(right)) {
+
+        if (right.outputSet().subtract(ij.references()).isEmpty() || isLocalEmptyRelation(right)) {


nit: I'd add a comment: "ij.references() are the join keys - the inline join doesn't add new columns. Since it preserves rows, it doesn't do anything and can be pruned."

Would this only happen when we are shadowing variables in the inline stats outside?

Example:

FROM employees | KEEP emp_no, salary, birth_date | EVAL orig_emp_no = emp_no | INLINE STATS emp_no = MAX(birth_date) BY salary | EVAL emp_no = birth_date

Or are there more cases?

Shadowing and DROP'ping. Maybe @astefan is aware of more cases.

This is about all use cases where what inline stats creates (meaning the aggs themselves) are "missing" after the inline stats command. This can be drop, keep [other columns and not the ones created by inline stats], eval, another stats (maybe others).

The other scenario (|| isLocalEmptyRelation(right)) comes from scenarios where the output of the right hand side is completely empty, meaning we don't need anything from what inline stats is using. In your specific query, for example, if I would do drop emp_no, salary then the inline stats itself can be pruned entirely. And I think that PruneEmptyPlans does that by creating a LocalRelation with an EMPTY supplier and the inline stats command itself gets to be pruned here.

The only subtlety is that even a pruned inline stats affects the column order if there is a BY clause and the grouping does not get dropped:

FROM employees | KEEP emp_no, languages, salary | INLINE STATS s = max(salary) BY languages | DROP s -> output: emp_no | salary | languages

The INLINE STATS moves the groupings to the right hand side, compared to just

FROM employees | KEEP emp_no, languages, salary

alex-spies · 2026-02-09T15:16:50Z

...ava/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFiltersTests.java

+     *   \_Filter[salary{f}#16 < 10000[INTEGER] AND salary{f}#16 > 10000[INTEGER]]
+     *     \_EsRelation[employees][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..]
     */
    public void testPushDown_ImpossibleFilter_PastInlineJoin() {


Btw, @GalLalouche 's golden tests can be used now - removing the toil required for writing/updating all the assertions below. It's a great time saver :)

I am an adept of seeing and checking what I see in the same file :-).

With #141082, the query that is being checked also gets put into the dir with the expected outcome, so you can just zip through the files during review. Worked great for me.

alex-spies · 2026-02-09T15:23:43Z

...ava/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineFiltersTests.java

            | INLINE STATS salary = AVG(salary) BY salary
            | WHERE salary > 10000


That's a sneaky one. Nice test!

ivancea · 2026-02-10T12:16:07Z

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

bpintea

What a nice simplification! Thanks Andrei.

ncordon

I left some questions, nothing major, it's mainly for my own understanding.

The change makes sense from my viewpoint because of two reasons: we are having into account variables that get used inside the right part of inline join that we didn't before and we are being less aggressive when pruning.

ncordon · 2026-02-10T17:30:08Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumnsTests.java

+    }
+
+    /*
+     * https://github.com/elastic/elasticsearch/issues/138283


Should we delete this line?

ncordon · 2026-02-11T00:50:42Z

...in/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumns.java

        var right = pruneColumns(ij.right(), used, true);
-        if (right.output().isEmpty() || isLocalEmptyRelation(right)) {
+
+        if (right.outputSet().subtract(ij.references()).isEmpty() || isLocalEmptyRelation(right)) {


Would this only happen when we are shadowing variables in the inline stats outside?

Example:

FROM employees | KEEP emp_no, salary, birth_date | EVAL orig_emp_no = emp_no | INLINE STATS emp_no = MAX(birth_date) BY salary | EVAL emp_no = birth_date

Or are there more cases?

ncordon · 2026-02-11T01:01:15Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumnsTests.java

+     *     | \_EsRelation[union_types_index*][!first_name, id{f}#11, !languages, !last_name, !sal..]
+     *     \_EsRelation[languages_lookup][LOOKUP][language_code{f}#13]
+     */
+    public void testPruneColumnsInProject_WithLookupJoin() {


Should we add some csv specs for these as well?

The csv spec tests are not relevant for the functionality of this PR change when it comes to Project pruning of union types related fields (see more here). Those fields were already not present in the output of these types of queries. What this PR does is something "invisible" in the output, meaning fields like !first_name, !languages, !last_name are not present in the Project sitting atop of EsRelation on the right-hand side of the LookupJoin node. Before this PR that Project had wrongly projecting those fields as well.

If you look closely at these tests, they are a bit different from the rest of tests regarding LogicalPlanOptimizer: they are directly calling PruneColumns rule (vs. running the full list of optimization rules batches).

ncordon · 2026-02-11T10:48:13Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

-        var eval = as(extract.child(), EvalExec.class);
-        var source = as(eval.child(), EsQueryExec.class);
+        var source = as(extract.child(), EsQueryExec.class);


Why has this changed?

Specifically I don't understand why the INLINE STATS prunning or the Project prunning affect this test

Because _EvalExec[[null[INTEGER] AS languages#21]] is not needed anymore, since it is not used afterwards. languages#21 is projected with \_ProjectExec[[emp_no{f}#18, languages{r}#21 AS language_code#7, first_name{f}#19]] but afterwards, languages#21 and/or language_code#7 is not re-used anywhere else. It is overiden by language_code{r}#12 that grok creates.

…139359_fix_take2

elasticsearchmachine · 2026-02-12T12:38:41Z

💔 Backport failed

Status	Branch	Result
❌	9.3	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 141056

Removes incorrect pruning in `inline stats` that lacked taking into consideration the fact that groupings can be multi-valued. Fixes elastic#139359 (comment) (grouping by an mv fields can lead to inconsistencies in certain queries) Fixes elastic#139359 Fixes elastic#140757 This also extracts a bunch of tests methods from the huge `LogicalPlanOptimizerTests` and moves them to a new test class - `PruneColumnsTests` which should include most of the tests that have in the spotlight the `PruneColumns` optimization rule. (cherry picked from commit 244538b)

* ESQL: remove incorrect inline stats pruning (#141056) Removes incorrect pruning in `inline stats` that lacked taking into consideration the fact that groupings can be multi-valued. Fixes #139359 (comment) (grouping by an mv fields can lead to inconsistencies in certain queries) Fixes #139359 Fixes #140757 This also extracts a bunch of tests methods from the huge `LogicalPlanOptimizerTests` and moves them to a new test class - `PruneColumnsTests` which should include most of the tests that have in the spotlight the `PruneColumns` optimization rule. (cherry picked from commit 244538b) * Update * Adapt the fix to 9.3.0

elasticsearchmachine added the v9.4.0 label Jan 21, 2026

astefan added 2 commits January 27, 2026 16:31

remove incorrect pruning and add tests

0f4810a

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

ab355ef

…139359_fix_take2r

astefan force-pushed the 139359_fix_take2 branch from 3739cc4 to ab355ef Compare January 27, 2026 14:34

astefan and others added 2 commits January 27, 2026 19:45

Merge branch 'main' into 139359_fix_take2

34be42c

Extract the PruneColumns tests in a separate test class

78489f4

astefan added >bug >tech debt :Analytics/ES|QL AKA ESQL labels Jan 30, 2026

Update docs/changelog/141056.yaml

88e4503

astefan added 3 commits January 30, 2026 12:27

More tests

1d573e0

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

79922f3

…139359_fix_take2

Merge branch '139359_fix_take2' of https://github.com/astefan/elastic…

22d1389

…search into 139359_fix_take2

astefan added v9.3.0 auto-backport Automatically create backport pull requests when merged labels Jan 30, 2026

astefan marked this pull request as ready for review January 30, 2026 10:34

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 30, 2026

astefan requested review from alex-spies and bpintea January 30, 2026 10:35

astefan commented Jan 30, 2026

View reviewed changes

Fix changelog

b6c9e4d

astefan mentioned this pull request Feb 4, 2026

ESQL: don't let grouping be pruned if there are still aggregates to compute #140814

Closed

alex-spies mentioned this pull request Feb 4, 2026

ESQL: WIP: simplify PruneColumns #140835

Closed

ncordon self-requested a review February 5, 2026 12:00

alex-spies approved these changes Feb 9, 2026

View reviewed changes

astefan mentioned this pull request Feb 10, 2026

ESQL: Fix INLINE STATS right side key column pruning on project #142215

Closed

ivancea reviewed Feb 10, 2026

View reviewed changes

bpintea approved these changes Feb 11, 2026

View reviewed changes

ncordon reviewed Feb 11, 2026

View reviewed changes

ncordon approved these changes Feb 11, 2026

View reviewed changes

astefan added 3 commits February 11, 2026 17:34

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

23f5f5e

…139359_fix_take2

Address reviews comments

e2bc0c5

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

584963c

…139359_fix_take2

astefan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 11, 2026

astefan added 2 commits February 12, 2026 11:56

More deterministic test

c71ac54

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

4f0072c

…139359_fix_take2

elasticsearchmachine merged commit 244538b into elastic:main Feb 12, 2026
35 checks passed

astefan deleted the 139359_fix_take2 branch February 12, 2026 12:38

elasticsearchmachine added the backport pending label Feb 12, 2026

astefan mentioned this pull request Feb 12, 2026

ESQL: remove incorrect inline stats pruning (#141056) #142389

Merged

astefan added v9.3.1 and removed v9.3.0 backport pending labels Feb 13, 2026

alex-spies mentioned this pull request Feb 16, 2026

ES|QL: illegal_argument_exception - "blocks is empty" #142473

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: remove incorrect inline stats pruning#141056

ESQL: remove incorrect inline stats pruning#141056
elasticsearchmachine merged 14 commits intoelastic:mainfrom
astefan:139359_fix_take2

astefan commented Jan 21, 2026 •

edited

Loading

elasticsearchmachine commented Jan 30, 2026

elasticsearchmachine commented Jan 30, 2026

astefan Jan 30, 2026

astefan Jan 30, 2026

astefan Jan 30, 2026

astefan Jan 30, 2026

bpintea Feb 11, 2026

alex-spies left a comment

alex-spies Feb 9, 2026

ncordon Feb 11, 2026

alex-spies Feb 11, 2026

astefan Feb 11, 2026

alex-spies Feb 12, 2026

alex-spies Feb 9, 2026

astefan Feb 11, 2026

alex-spies Feb 12, 2026

alex-spies Feb 9, 2026

ivancea Feb 10, 2026

bpintea left a comment

ncordon left a comment

ncordon Feb 10, 2026

ncordon Feb 11, 2026

ncordon Feb 11, 2026

astefan Feb 11, 2026

astefan Feb 11, 2026

ncordon Feb 11, 2026

ncordon Feb 11, 2026

astefan Feb 11, 2026

Uh oh!

elasticsearchmachine commented Feb 12, 2026

Labels

6 participants


		public class PruneColumnsTests extends AbstractLogicalPlanOptimizerTests {

		public void testPruneUnusedEval() {

+                      }
+                  }
+                  /*

		\| INLINE STATS salary = AVG(salary) BY salary
		\| WHERE salary > 10000

Conversation

astefan commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Jan 30, 2026

elasticsearchmachine commented Jan 30, 2026

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

alex-spies left a 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

Choose a reason for hiding this comment

ncordon left a 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

Uh oh!

elasticsearchmachine commented Feb 12, 2026

💔 Backport failed

Labels

6 participants

astefan commented Jan 21, 2026 •

edited

Loading