Introduce execution location marker for better handling of remote/local compatibility #132205

smalyshev · 2025-07-30T20:14:39Z

Introduce ExecutedOn interface which provides the means to mark the plan as executed only on coordinator or only remotely. This will allow to generalize checks for the constructs that can not be run after coordinator-only constructs. It also allows to identify the constructs currently not allowed before remote-only constructs:

STATS
INLINESTATS
FORK
RERANK
COMPLETION
CHANGE_POINT
coordinator ENRICH

…al compatibility

Copilot

~~

alex-spies

This is very nice. I think we should go with this approach.

To be complete, I think SORT/TopN and LIMIT normally also deserve to be marked as ExecutesOn.COORDINATOR; remote ENRICH is special in that it can't deal with stuff that requires the coordinator, except topn and limit specifically - but remote LOOKUP JOIN doesn't deal well with any coordinator-only nodes, and would also profit from using this abstraction.

I think LIMITs are a very interesting case this could cover nicely; more specifically LIMITs before remote LOOKUP JOINs. They are currently considered invalid if a user puts them there, but not if our optimizations put them there - because we know that the optimization only duplicates a limit rather than pushing it fully past LOOKUP JOIN.

Marking the duplicated LIMIT as ANY while other limits are generally marked as COORDINATOR would make the validation less brittle in case that other optimizations end up putting LIMITs upstream from remote LOOKUP JOINs, or other surrogate plans end up creating LIMITs somehow.

alex-spies · 2025-07-31T06:35:11Z

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

+    implements
+        PostAnalysisPlanVerificationAware,
+        TelemetryAware,
+        PipelineBreaker,


Marking FORK as pipeline breaker would become obsolete with the new interface, right?

Maybe the whole PipelineBreaker interface gets obsolete; or maybe not, because the original pipeline breakers (TopN, Limit, Aggs) are special in that they get "duplicated" into the fragment if they are the first node on the coordinator.

Possibly? I think PipelineBreaker may have its uses but it's both too broad and too inexact for what we need here, that's why I want to introduce a narrow-targeted interface.

I think introducing the narrow interface is nice :)

But Fork wasn't marked as pipeline breaker until #131945, and there we discussed that marking it as pipeline breaker could lead to confusion in other places that use that interface in a more specific way.

I'm proposing we undo that change and just mark Fork as ExecutesOn.Coordinator - marking as pipeline breaker shouldn't be required anymore,right?

I think you are right, it doesn't really need to be marked as breaker anymore now.

smalyshev · 2025-08-12T16:44:37Z

...ain/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceLimitAndSortAsTopN.java

        LogicalPlan p = plan;
        if (plan.child() instanceof OrderBy o) {
-            p = new TopN(plan.source(), o.child(), o.order(), plan.limit());
+            p = new TopN(o.source(), o.child(), o.order(), plan.limit());


I think this is just a bug - no reason why TopN source has to come from limit, OrderBy is much better IMO.

elasticsearchmachine · 2025-08-13T07:06:01Z

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

alex-spies

Looks good! Thanks for improving our abstractions, I think that helps reason about the language and planning more cleanly. Nice!

Only minor comments from my side; go ahead at your own discretion, please!

alex-spies · 2025-08-14T16:47:34Z

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

+        PostAnalysisVerificationAware,
+        TelemetryAware,
+        SortAgnostic,
+        PipelineBreaker,


Thought: Hmm, maybe PipelineBreaker should inherit from ExecutesOn.Coordinator?

I am not sure yet... Need to think about it a bit more to have a good answer for this.

alex-spies · 2025-08-14T16:52:10Z

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

+    implements
+        PostAnalysisPlanVerificationAware,
+        TelemetryAware,
+        PipelineBreaker,


I think introducing the narrow interface is nice :)

But Fork wasn't marked as pipeline breaker until #131945, and there we discussed that marking it as pipeline breaker could lead to confusion in other places that use that interface in a more specific way.

I'm proposing we undo that change and just mark Fork as ExecutesOn.Coordinator - marking as pipeline breaker shouldn't be required anymore,right?

alex-spies · 2025-08-14T16:54:35Z

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

+                return; // skip myself
+            }
+            if (u instanceof Limit) {
+                // Limit is ok because it can be moved in by the optimizer


Let's maybe comment that we check for upstream limits before optimization and thus can be sure that any limits encounter now were duplicated and have a corresponding downstream limit that ensures correctness.

* upstream/main: (32 commits) Speed up loading keyword fields with index sorts (elastic#132950) Mute org.elasticsearch.index.mapper.LongFieldMapperTests testSyntheticSourceWithTranslogSnapshot elastic#132964 Simplify EsqlSession (elastic#132848) Implement WriteLoadConstraintDecider#canAllocate (elastic#132041) Mute org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT test {p0=search/400_synthetic_source/_doc_count} elastic#132965 Switch to PR-based benchmark pipeline defined in ES repo (elastic#132941) Breakdown undesired allocations by shard routing role (elastic#132235) Implement v_magnitude function (elastic#132765) Introduce execution location marker for better handling of remote/local compatibility (elastic#132205) Mute org.elasticsearch.cluster.ClusterInfoServiceIT testMaxQueueLatenciesInClusterInfo elastic#132957 Unmuting simulate index data stream mapping overrides yaml rest test (elastic#132946) Remove CrossClusterCancellationIT.createLocalIndex() (elastic#132952) Mute org.elasticsearch.index.mapper.LongFieldMapperTests testFetch elastic#132956 Fix failing UT by adding a required capability (elastic#132947) Precompute the BitsetCacheKey hashCode (elastic#132875) Adding simulate ingest effective mapping (elastic#132833) Mute org.elasticsearch.index.mapper.LongFieldMapperTests testFetchMany elastic#132948 Rename skipping logic to remove hard link to skip_unavailable (elastic#132861) Store ignored source in unique stored fields per entry (elastic#132142) Add random tests with match_only_text multi-field (elastic#132380) ...

…al compatibility (elastic#132205) * Introduce execution location marker for better handling of remote/local compatibility

…-stats * upstream/main: (36 commits) Fix reproducability of builds against Java EA versions (elastic#132847) Speed up loading keyword fields with index sorts (elastic#132950) Mute org.elasticsearch.index.mapper.LongFieldMapperTests testSyntheticSourceWithTranslogSnapshot elastic#132964 Simplify EsqlSession (elastic#132848) Implement WriteLoadConstraintDecider#canAllocate (elastic#132041) Mute org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT test {p0=search/400_synthetic_source/_doc_count} elastic#132965 Switch to PR-based benchmark pipeline defined in ES repo (elastic#132941) Breakdown undesired allocations by shard routing role (elastic#132235) Implement v_magnitude function (elastic#132765) Introduce execution location marker for better handling of remote/local compatibility (elastic#132205) Mute org.elasticsearch.cluster.ClusterInfoServiceIT testMaxQueueLatenciesInClusterInfo elastic#132957 Unmuting simulate index data stream mapping overrides yaml rest test (elastic#132946) Remove CrossClusterCancellationIT.createLocalIndex() (elastic#132952) Mute org.elasticsearch.index.mapper.LongFieldMapperTests testFetch elastic#132956 Fix failing UT by adding a required capability (elastic#132947) Precompute the BitsetCacheKey hashCode (elastic#132875) Adding simulate ingest effective mapping (elastic#132833) Mute org.elasticsearch.index.mapper.LongFieldMapperTests testFetchMany elastic#132948 Rename skipping logic to remove hard link to skip_unavailable (elastic#132861) Store ignored source in unique stored fields per entry (elastic#132142) ...

Introduce execution location marker for better handling of remote/loc…

2c051b5

…al compatibility

elasticsearchmachine added the v9.2.0 label Jul 30, 2025

refactor

170d83c

smalyshev requested review from alex-spies and Copilot and removed request for Copilot July 30, 2025 22:57

Copilot AI reviewed Jul 30, 2025

View reviewed changes

[CI] Auto commit changes from spotless

411a2ed

elastic deleted a comment from Copilot AI Jul 30, 2025

alex-spies reviewed Jul 31, 2025

View reviewed changes

smalyshev added 4 commits July 31, 2025 12:06

Merge branch 'main' into executes-on

ae7a7ce

More fixes

9e4be57

typo

37600f0

Merge branch 'main' into executes-on

f7f5e9b

smalyshev force-pushed the executes-on branch from 4baa612 to f7f5e9b Compare August 11, 2025 18:15

smalyshev added 2 commits August 11, 2025 16:05

Move checks to post-optimizer

841a22e

Add limit handling

acb933c

smalyshev commented Aug 12, 2025

View reviewed changes

elasticsearchmachine and others added 4 commits August 12, 2025 16:51

[CI] Auto commit changes from spotless

0535258

Merge branch 'main' into executes-on

e67924c

One more test case

8413dbc

[CI] Auto commit changes from spotless

997b9bd

smalyshev marked this pull request as ready for review August 13, 2025 07:05

smalyshev added :Analytics/ES|QL AKA ESQL >refactoring >non-issue labels Aug 13, 2025

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

smalyshev requested a review from alex-spies August 13, 2025 07:06

alex-spies approved these changes Aug 14, 2025

View reviewed changes

smalyshev and others added 4 commits August 14, 2025 12:38

Merge branch 'main' into executes-on

61fe122

review feedback

e00abb5

Merge branch 'main' into executes-on

6ff5827

Merge branch 'main' into executes-on

2e2f953

smalyshev merged commit 37b28e4 into elastic:main Aug 14, 2025
33 checks passed

smalyshev deleted the executes-on branch August 14, 2025 22:15

alex-spies mentioned this pull request Aug 18, 2025

ESQL: Refine PipelineBreaker interface #131507

Closed

Introduce execution location marker for better handling of remote/local compatibility #132205

Introduce execution location marker for better handling of remote/local compatibility #132205

Uh oh!

Conversation

smalyshev commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

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!

elasticsearchmachine commented Aug 13, 2025

Uh oh!

alex-spies left a comment

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

Milestone

Development

Uh oh!

3 participants

smalyshev commented Jul 30, 2025 •

edited

Loading

Copilot AI left a comment •

edited by smalyshev

Loading