ESQL: Fix ExchangeSinkExec output type by GalLalouche · Pull Request #142489 · elastic/elasticsearch

GalLalouche · 2026-02-13T16:14:43Z

This PR fixes ExchangeSinkExec claiming to return the wrong output. This doesn't actually affect production, since the operator outputs whatever it receives anyway, but it's confusing, and looks bad in golden tests as well.
Also added a plan verification that only runs during tests to ensure this kind of thing doesn't happen again.

Resolves #141654.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlannerUtils.java

elasticsearchmachine · 2026-02-13T16:17:44Z

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

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java

change_commits.sh

repeat.sh

...enTests/testTopNWithMissingSortField/local_reduce_physical_optimization_data_driver.expected

alex-spies · 2026-02-23T18:02:35Z

...enTests/testTopNWithMissingSortField/local_reduce_physical_optimization_data_driver.expected

-ExchangeSinkExec[[emp_no{f}, hire_date{f}, salary{f}],false]
-\_ProjectExec[[_doc{f}, birth_date{r} AS hire_date]]
-  \_EvalExec[[null[DATETIME] AS birth_date]]
+ExchangeSinkExec[[_doc{f}, hire_date{r}],false]


thought: With and without my test fix, we have an interesting situation here: the exchange doesn't return a field attribute anymore, but a reference attribute (because we intentionally shadowed the hire_date with a null constant from an EVAL that was inserted for the missing field.

This is correct, but means we cannot assume that attribute type remains consistent for an exchange after optimization.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlannerUtils.java

alex-spies

Ok, done reviewing now. The productive changes look alright, ~~except for the automatic "fix my exchanges, pls" in ComputeService.java. This shouldn't really be necessary and looks like a leftover.~~

...enTests/testTopNWithMissingSortField/local_reduce_physical_optimization_data_driver.expected

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java

The exchange sinks are already being updated in ComputeService.java. Let's keep the exchange sink update constrained to one place.

The consistency checks in PlannerUtils#reductionPlan are already checking that late materialization planning didn't mess up the data and reduce plans' consistency.

alex-spies

Ok, I simplified the PR a bit. If the tests still pass, we should be good to go.

alex-spies

Ok, in addition to the FragmentExec "lying" about its output, I can see that TimeSeriesAggregateExec has some peculiarities as well.

~~I'd like to try and fix those before merging this, so we don't go and artificially declare factually inconsistent plans as consistent.~~ Update: changed my mind; getting the exchanges consistent for aggs is surprisingly complex, given how we generate intermediate attributes. I think it's not worth fixing for now.

Found 1 problem
line -1:-1: Plan [TimeSeriesAggregateExec[[_tsid{m}#54, bucket(@timestamp, 1 hour){r}#55],[MIN($$cpu$MINOVERTIME$min_over_time(cpu){f$}#63,true[BOOLEAN],
PT0S[TIME_DURATION]) AS MINOVERTIME_$1#64, DIMENSIONVALUES(cluster{f}#60,true[BOOLEAN],PT0S[TIME_DURATION]) AS cluster#65, bucket(@timestamp, 1 hour){r}#55],
INTERMEDIATE,[_tsid{m}#54, bucket(@timestamp, 1 hour){r}#55, $$MINOVERTIME_$1$min{r}#66, $$MINOVERTIME_$1$seen{
r}#67, $$cluster$values{r}#68],66,BUCKET(@timestamp{f}#59,PT1H[TIME_DURATION])]] optimized incorrectly due to missing references [$$MINOVERTIME_$1$min{r}#66, $$MINOVERTIME_$1$seen{r}#67, $$cluster$values{r}#68]
java.lang.IllegalStateException: failed for plan
ExchangeSinkExec[[_tsid{m}#54, bucket(@timestamp, 1 hour){r}#55, $$MINOVERTIME_$1$min{r}#66, $$MINOVERTIME_$1$seen{r}#67, $$clu
ster$values{r}#68],true]
\_TimeSeriesAggregateExec[[_tsid{m}#54, bucket(@timestamp, 1 hour){r}#55],[MIN($$cpu$MINOVERTIME$min_over_time(cpu){f$}#63,true[BOOLEAN],
PT0S[TIME_DURATION]) AS MINOVERTIME_$1#64, DIMENSIONVALUES(cluster{f}#60,true[BOOLEAN],PT0S[TIME_DURATION]) AS cluster#65, bucket(@timestamp, 1 hour){r}#55],
INTERMEDIATE,[_tsid{m}#54, bucket(@timestamp, 1 hour){r}#55, $$MINOVERTIME_$1$min{r}#66, $$MINOVERTIME_$1$seen{
r}#67, $$cluster$values{r}#68],66,BUCKET(@timestamp{f}#59,PT1H[TIME_DURATION])]
  \_ExchangeSourceExec[[_tsid{m}#54, bucket(@timestamp, 1 hour){r}#55, $$MINOVERTIME_$1$min{r}#56, $$MINOVERTIME_$1$seen{r}#57, $$clu
ster$values{r}#58],true]

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java

That actually genuinely needs to stay untouched.

This reverts commit f139867.

Only fix late materialization planning. Only check consistency for non-agg plans; aggs remain inconsistent for now.

alex-spies · 2026-02-23T22:06:27Z

Ok, I ended up reducing scope a little. There was a workaround in place to deal with aggs, I removed it, but excluded aggs from the consistency check for exchanges. The intermediate attributes for aggs and time series aggs make this just too complicated to be worth the effort.

I reduced the exchange sink fix to just the known broken sinks in case of late materialization.
I hardened the consistency check to ensure the overall output didn't change and the data driver outputs what the reduce driver requires.
I fixed a test that was considering too many fields as missing.

Hope CI likes this now.

alex-spies · 2026-02-23T22:14:37Z

...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/LateMaterializationPlanner.java

        FragmentExec updatedFragmentExec = fragmentExec.withFragment(updatedFragment);
-        // TODO This ignores the possible change in output, see #141654
-        ExchangeSinkExec updatedDataPlan = originalPlan.replaceChild(updatedFragmentExec);
+        ExchangeSinkExec updatedDataPlan = originalPlan.replaceChildAndUpdateOutput(updatedFragmentExec);


Main fix here.

...src/test/java/org/elasticsearch/xpack/esql/plugin/LateMaterializationPlannerGoldenTests.java

…n_node_reduce

* Fix ExchangeSinkExec output for node-level late materialization. * Add a consistency check for node-level reduction and data node plans. --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

GalLalouche · 2026-03-03T09:51:30Z

@alex-spies Thanks a lot for chaperoning this to production!

ESQL: Fix ExchangeSinkExec output type

de8e2a5

GalLalouche requested a review from alex-spies February 13, 2026 16:14

elasticsearchmachine added v9.4.0 needs:triage Requires assignment of a team area label labels Feb 13, 2026

GalLalouche commented Feb 13, 2026

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/PlannerUtils.java Show resolved Hide resolved

GalLalouche added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL >refactoring and removed >non-issue labels Feb 13, 2026

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Feb 13, 2026

GalLalouche marked this pull request as draft February 13, 2026 16:18

GalLalouche added 2 commits February 13, 2026 18:40

Fix SpecIT failing

3942dd0

Fix source input

befbc1a

GalLalouche marked this pull request as ready for review February 13, 2026 21:24

GalLalouche commented Feb 13, 2026

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java Outdated Show resolved Hide resolved

GalLalouche force-pushed the fix/exchange_exec_in_node_reduce branch from 15d0c04 to 932cf31 Compare February 13, 2026 21:47

idegtiarenko reviewed Feb 23, 2026

View reviewed changes

change_commits.sh Outdated Show resolved Hide resolved

idegtiarenko reviewed Feb 23, 2026

View reviewed changes

repeat.sh Outdated Show resolved Hide resolved

Fix source input

6ed1ae0

GalLalouche force-pushed the fix/exchange_exec_in_node_reduce branch from 932cf31 to 6ed1ae0 Compare February 23, 2026 08:56

GalLalouche and others added 3 commits February 23, 2026 13:05

Merge branch 'main' into fix/exchange_exec_in_node_reduce

1d6a36a

Merge branch 'main' into fix/exchange_exec_in_node_reduce

613e313

Fix testTopNWithMissingSortField

3ce094a

alex-spies reviewed Feb 23, 2026

View reviewed changes

...enTests/testTopNWithMissingSortField/local_reduce_physical_optimization_data_driver.expected Outdated Show resolved Hide resolved

Move consistency check to top of localPlan

edfe402

alex-spies reviewed Feb 23, 2026

View reviewed changes

Revert LateMaterializationPlanner changes

41137ef

The exchange sinks are already being updated in ComputeService.java. Let's keep the exchange sink update constrained to one place.

alex-spies added 3 commits February 23, 2026 20:18

Remove added assertion in PlannerUtils#localPlan

5f5596b

The consistency checks in PlannerUtils#reductionPlan are already checking that late materialization planning didn't mess up the data and reduce plans' consistency.

Simplify and harden ComputeService#reductionPlan

ccc33df

Simplify some more

4a9a4f1

alex-spies approved these changes Feb 23, 2026

View reviewed changes

alex-spies requested a review from idegtiarenko February 23, 2026 20:17

alex-spies reviewed Feb 23, 2026

View reviewed changes

Fix FragmentExec#output

f139867

alex-spies reviewed Feb 23, 2026

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java Outdated Show resolved Hide resolved

alex-spies added 3 commits February 23, 2026 22:27

Do not update reduce plan output

aea8e5f

That actually genuinely needs to stay untouched.

Revert "Fix FragmentExec#output"

747f5a3

This reverts commit f139867.

Reduce scope

a5c9b37

Only fix late materialization planning. Only check consistency for non-agg plans; aggs remain inconsistent for now.

[CI] Auto commit changes from spotless

27ac3a2

alex-spies reviewed Feb 23, 2026

View reviewed changes

Also exclude TimeSeriesAggregates

34bf0b5

idegtiarenko approved these changes Feb 24, 2026

View reviewed changes

idegtiarenko reviewed Feb 24, 2026

View reviewed changes

...src/test/java/org/elasticsearch/xpack/esql/plugin/LateMaterializationPlannerGoldenTests.java Outdated Show resolved Hide resolved

idegtiarenko reviewed Feb 24, 2026

View reviewed changes

...src/test/java/org/elasticsearch/xpack/esql/plugin/LateMaterializationPlannerGoldenTests.java Outdated Show resolved Hide resolved

idegtiarenko approved these changes Feb 24, 2026

View reviewed changes

alex-spies self-assigned this Feb 25, 2026

alex-spies added 3 commits February 25, 2026 13:28

Merge remote-tracking branch 'upstream/main' into fix/exchange_exec_i…

0303691

…n_node_reduce

Skip check for MetricsInfo

c80a7a8

Address Ievgen's comments

633e8c7

alex-spies merged commit 3e34e66 into elastic:main Feb 25, 2026
35 checks passed

GalLalouche mentioned this pull request Mar 3, 2026

ESQL: Add name IDs to golden tests and fix synthetic names #143450

Merged

Conversation

GalLalouche commented Feb 13, 2026

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-spies Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-spies 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies 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!

Uh oh!

alex-spies commented Feb 23, 2026

Uh oh!

alex-spies Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GalLalouche commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alex-spies left a comment •

edited

Loading

alex-spies left a comment •

edited

Loading