Replace remote Enrich hack with proper handling of remote Enrich #134967

smalyshev · 2025-09-17T23:16:56Z

The remote Enrich hack and the verification workaround that it necessitates are removed
Limit with remote Enrich is handled by duplicating the limits around Enrich
TopN with remote Enrich is handled by duplicating around Enrich and adjusting projections
Introduced CardinalityPreserving interface to mark nodes that preserve cardinality
Introduced the capability to run optimizations only on coordinator plan (CoordinatorOnly interface)
Added post-optimizer check that ensures remote Enrich is not moved to the coordinator plan

TODO: this does not fix the issue of local (not remote) Lookup Join always being forced on the coordinator by Limits, we may address this in the followup, this one is meaty enough as it is.

Closes #115897
Fixes #118531

alex-spies

I had a first look at the new optimizer rules and left some comments.

...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/HoistRemoteEnrichTopN.java

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

...c/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/HoistRemoteEnrichLimit.java

elasticsearchmachine · 2025-09-26T01:46:20Z

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

alex-spies

Done a first pass - need another one. I'm like halfway through it, but haven't fully thought through the added optimizer rules, yet.

I noticed that this doesn't add new optimizer tests, yet. That's something I think we should really add before this can be merged; I'd go and try really hard to find edge cases where the hoisting mechanism, the new local-only topn/limit etc. may break.

(The generative tests will also do that to some extent (let's make sure they actually create remote-only enrichs!), but it's good to have a human-made baseline.)

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

alex-spies · 2025-10-02T16:52:37Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

        );
    }

    protected static Batch<LogicalPlan> operators(boolean local) {


nit: Hey, we have 3 mechanisms now that convey whether a rule should be run on the coordinator, the data node, or behave differently depending on global/local: this boolean parameter, the CoordinatorOnly interface, and the method LocalLogicalPlanOptimizer#localOperators which does some special filtering on its own.

I like the CoordinatorOnly interface, but'd prefer we settle on 1 mechanism if possible.

Maybe rather than a CoordinatorOnly interface, we should have a DifferentOnDataNode interface (yes, the name is horrible, I didn't have much time to think :D ) with a method localVersion (or so) that either gives the data node version of the optimizer rule (applies to CombineProjections and PropagateEmptyRelation) or null (applies to all rules marked CoordinatorOnly in this PR).

I made LocalLogicalPlanOptimizer#localOperators respect the CoordinatorOnly marker, so that's the same thing now, except for rule that work on both sides, but differently (PropagateEmptyRelation). I agree that it could be refined further but not sure the best way yet.

If you don't mind, I'd like to push a small refactoring that consolidates the 3 methods for adjusting rules to the local optimizer into one.

I pushed 2c67a57. Feel free to revert if you don't like it, though :)

...sql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/CombineLimitTopN.java

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

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

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

alex-spies · 2025-10-03T13:01:13Z

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

 import java.util.Objects;

-public class Limit extends UnaryPlan implements TelemetryAware, PipelineBreaker {
+public class Limit extends UnaryPlan implements TelemetryAware, PipelineBreaker, ExecutesOn {


It's a little confusing now that this is PipelineBreaker but can still execute on ANY sometimes.

Out of scope, but maybe we'll want to reconcile the two interfaces later; PipelineBreaker could be another enum variant; so we'd have "executes anywhere", "executes only on data nodes", "breaks the pipeline if we're not already on the coordinator", "runs only on the coordinator".

The thing is, commands like LIMIT are "sometimes pipeline breakers" now, but we don't have an interface like that. It may be a good idea to think about how to refine that, but I don't think in this patch.

Could you maybe attempt that in a follow-up, please? Specifically, trying to consolidate the ExecutesOn and PipelineBreaker interfaces.

I don't want us to spend too many cycles on this; but the two interfaces have significant overlap in their meaning and this looks like one of those concepts that we best simplify if possible, otherwise new devs that start hacking on ESQL will have a hard time understanding this.

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

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/logical/TopNSerializationTests.java

alex-spies · 2025-10-07T12:03:52Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java

-            return duplicateLimitAsFirstGrandchild(limit);
+            // We use withLocal = false because if we have a remote join it will be forced into the fragment by the mapper anyway,
+            // And the verifier checks that there are no non-synthetic limits before the join.
+            // TODO: However, this means that the non-remote join will be always forced on the coordinator. We may want to revisit this.


Non-remote join on the coordinator is fine and probably more efficient in many (most?) cases. To perform a lookup join, we reach out to the nodes holding a replica of the lookup index (single sharded); that doesn't seem like something where parallelizing it across nodes will make it faster - but it would definitely do more work overall.

alex-spies · 2025-10-07T12:12:05Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java

            // left-hand side, so adding a limit in there would lead to the right-hand side work on incomplete data.
            // To avoid repeating this infinitely, we have to set duplicated = true.
-            return duplicateLimitAsFirstGrandchild(limit);
+            // We use withLocal = false because if we have a remote join it will be forced into the fragment by the mapper anyway,


To be consistent, couldn't we make withLocal depend on whether this is a remote join or not?

The current remote lookup join is built on trust :) Trust, that our optimizer won't stuff a real pipeline breaker below it if it wasn't there before. Formally, we're violating this when we duplicate a limit past a join, because we know that the mapper will treat this as a local limit anyway. But until it hits a remote lookup join, the mapper will actually behave as if the upstream limit it already found is a pipeline breaker.

We could make this less hacky if the mapper knew that it doesn't have to treat the limit as pipeline breaker to begin with. In fact, this special casing for joins in the mapper might become obsolete if we set the correct local value on the limit, no? Then, the next case should trigger automatically; and the special handling of duplicated limits would be fully contained in PushDownAndCombineLimits.

alex-spies · 2025-10-07T12:17:35Z

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

    @Override
    public void postOptimizationVerification(Failures failures) {
        if (this.mode == Mode.REMOTE) {


Because we declared Enrich to be PostOptimizationVerificationAware.CoordinatorOnly, shouldn't this have become obsolete?

alex-spies · 2025-10-07T12:26:44Z

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

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


nit: Wouldn't it be more corect if this was

Suggested change

p = new TopN(o.source(), o.child(), o.order(), plan.limit(), false);

p = new TopN(o.source(), o.child(), o.order(), plan.limit(), plan.local() && o.local());

?

This rule does run on the coordinator and it currently declares any topn it creates as pipeline breaking. That's currently okay because we don't create local SORTs. But when reading this in isolation, I don't know that if I'm not already familiar with this code :)

Maybe let's make this either more correct on its own, or add a comment for future devs that stumble upon this.

alex-spies · 2025-10-07T12:33:18Z

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

 import java.util.Objects;

-public class Limit extends UnaryPlan implements TelemetryAware, PipelineBreaker {
+public class Limit extends UnaryPlan implements TelemetryAware, PipelineBreaker, ExecutesOn {


Could you maybe attempt that in a follow-up, please? Specifically, trying to consolidate the ExecutesOn and PipelineBreaker interfaces.

I don't want us to spend too many cycles on this; but the two interfaces have significant overlap in their meaning and this looks like one of those concepts that we best simplify if possible, otherwise new devs that start hacking on ESQL will have a hard time understanding this.

alex-spies · 2025-10-07T13:22:21Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

        );
    }

    protected static Batch<LogicalPlan> operators(boolean local) {


If you don't mind, I'd like to push a small refactoring that consolidates the 3 methods for adjusting rules to the local optimizer into one.

smalyshev and others added 11 commits September 4, 2025 13:56

Add check that remote enrich stays on remote side

d777561

Merge branch 'main' into remote-enrich-check

a93438f

Make it more generic

b6244e5

[CI] Auto commit changes from spotless

a787ae8

Verifier refactor

6d638be

Merge branch 'main' into remote-enrich-check

95ef823

Merge branch 'main' into remote-enrich-check

2490f60

Fix the fix for elastic#118531

4e16026

Merge branch 'main' into remote-enrich-check

e3324c0

Remove old hacks

dd7dd79

Replace remote Enrich hack with proper handling of remote Enrich

31df078

elasticsearchmachine added the v9.2.0 label Sep 17, 2025

Don't stop for ANY enrich

8dde748

smalyshev changed the title ~~Replace remote Enrich hack with proper handling of remote Enrich~~ [WIP] Replace remote Enrich hack with proper handling of remote Enrich Sep 18, 2025

smalyshev and others added 13 commits September 18, 2025 14:27

Add TopN handling

d053fe4

Merge branch 'main' into entich-remote-unhack

9b185e8

Oops, forgot the file

93c827d

fix multiple topNs

a78a2c8

fix tests

ea421ca

test fixes

e70043e

Ban Limit + MvExpand before remote Enrich

9edcd72

Update docs/changelog/135051.yaml

7c6c9b1

spotless

3f2e2e9

Move check pre optimizer

3d909ce

Merge branch 'remote-enrich-limit' into entich-remote-unhack

ddc5815

fix tests

587ae4c

[CI] Update transport version definitions

f9686e5

alex-spies reviewed Sep 22, 2025

View reviewed changes

smalyshev and others added 2 commits September 22, 2025 12:06

test fixes

30146a1

[CI] Update transport version definitions

8dc2101

smalyshev added 4 commits September 25, 2025 08:30

Merge branch 'main' into entich-remote-unhack

3ab2bf7

cleanup

15f9cbf

Cleanups

616021d

clean test

255bce5

smalyshev requested a review from luigidellaquila September 25, 2025 16:01

smalyshev added 2 commits September 25, 2025 11:48

moar tests

c8a109b

oops typo

7b28e9f

smalyshev changed the title ~~[WIP] Replace remote Enrich hack with proper handling of remote Enrich~~ Replace remote Enrich hack with proper handling of remote Enrich Sep 25, 2025

Merge branch 'main' into entich-remote-unhack

3af3540

smalyshev marked this pull request as ready for review September 26, 2025 01:45

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

smalyshev requested a review from alex-spies September 29, 2025 15:54

elasticsearchmachine added v9.3.0 and removed v9.2.0 labels Oct 2, 2025

alex-spies reviewed Oct 3, 2025

View reviewed changes

smalyshev added v9.2.0 auto-backport Automatically create backport pull requests when merged labels Oct 3, 2025

smalyshev and others added 11 commits October 3, 2025 11:42

Merge branch 'main' into entich-remote-unhack

00bc8b7

Feedback, pass 1

6df9352

More feedback

3f58415

Remove limit optimization - data node optimizer will take care of it

8c0c305

moar tests

2145a40

avoid forbidden lore

74c8a9d

More tests

69c2225

TopN tests

e9160a5

Merge branch 'main' into entich-remote-unhack

9b40f1d

fix test

4c052ec

Refine CoordinatorOnly interface for rules

2c67a57

alex-spies reviewed Oct 7, 2025

View reviewed changes

	p = new TopN(o.source(), o.child(), o.order(), plan.limit(), false);
	p = new TopN(o.source(), o.child(), o.order(), plan.limit(), plan.local() && o.local());

Replace remote Enrich hack with proper handling of remote Enrich #134967

Are you sure you want to change the base?

Replace remote Enrich hack with proper handling of remote Enrich #134967

Conversation

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

Uh oh!

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 26, 2025

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

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!

smalyshev commented Sep 17, 2025 •

edited

Loading