ESQL: Disallow remote enrich after lu join #131426

alex-spies · 2025-07-17T09:55:17Z

Due to how remote ENRICH is planned, it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH.

We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows.

For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway.

There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment here and my draft #131286 for enabling this.)

elasticsearchmachine · 2025-07-17T09:55:43Z

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

elasticsearchmachine · 2025-07-17T09:57:25Z

Hi @alex-spies, I've created a changelog YAML for you.

This was broken before: a coordinator-only ENRICH in one branch and a remote-only ENRICH in the other could be (wrongly) considered invalid depending on the order of the fork branches.

This interprets LOOKUP JOIN as a remote-only LOOKUP JOIN which is more restricted.

luigidellaquila

Thanks Alex, LGTM.

I left a few comments, especially about improvements we can do in test coverage.

One more thing we can do is add ENRICH _remote: to the generative tests, but that's something we can do in a follow-up PR

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

luigidellaquila · 2025-07-17T16:41:13Z

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

+            return;
+        }
+
+        // TODO: shouldn't we also include FORK? Everything downstream from FORK should be coordinator-only.


I'm not completely sure, but if FORK needs merging on the coordinator, then we definitely want it here.

Why does FORK need to be on coordinator always? I mean, if that's currently the case then yes, we must respect it here and provide the check for it, but if the clauses inside FORK are not coordinator-only (like STATS) in general why there should be the coordinator stage? Maybe I don't understand the semantics of it yet.

Since FORK is 9.1+ though, it may be easier to take care of it in a separate pull. And maybe even not mention FORK if we're merging it down to 8.18, otherwise somebody reading 8.18 branch code could be quite confused.

I'm not familiar enough with FORK to make a quick call, so I think it's best if that is handled separately and I opened #131445 to track it.

That said, the javadoc here implies that the nodes after a FORK all execute on the coordinator, making FORK actually a pipeline breaker.

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

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

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

luigidellaquila · 2025-07-17T16:56:52Z

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

            }
        });
+
+        if (aggregate[0]) {


If we add new commands (FORK?) to this list, maybe we can start considering the creation of a flag interface to identify them (similar to current PipelineBreaker, but it's not exactly the same thing), and avoid the list of instanceof

I think having an interface that identifies the "not remotable" components would be nice, however it may be tricky because some operations may depend on options (like Enrich itself which can be "coordinator" or "remote" or "any"). Still, it might save some effort for simpler operations.

Agree. We've come across this when working on remote LOOKUP JOINs, already, see my comment here. I think we may want to refine the concept of pipeline breakers as part of improving remote ENRICH in #115897. I'll add a comment there to track these observations :)

smalyshev · 2025-07-17T17:36:44Z

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

+            return;
+        }
+
+        // TODO: shouldn't we also include FORK? Everything downstream from FORK should be coordinator-only.


Why does FORK need to be on coordinator always? I mean, if that's currently the case then yes, we must respect it here and provide the check for it, but if the clauses inside FORK are not coordinator-only (like STATS) in general why there should be the coordinator stage? Maybe I don't understand the semantics of it yet.

smalyshev · 2025-07-17T19:06:36Z

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

            }
        });
+
+        if (aggregate[0]) {


I think having an interface that identifies the "not remotable" components would be nice, however it may be tricky because some operations may depend on options (like Enrich itself which can be "coordinator" or "remote" or "any"). Still, it might save some effort for simpler operations.

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

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

smalyshev · 2025-07-17T19:17:12Z

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

+            return;
+        }
+
+        // TODO: shouldn't we also include FORK? Everything downstream from FORK should be coordinator-only.


Since FORK is 9.1+ though, it may be easier to take care of it in a separate pull. And maybe even not mention FORK if we're merging it down to 8.18, otherwise somebody reading 8.18 branch code could be quite confused.

smalyshev · 2025-07-17T19:19:15Z

LGTM in general, added some thoughts & nitpicks inline.

Randomly use a lookup index that doesn't shadow the columns added by the ENRICH. This could be relevant if we at some point allow LU JOIN after remote ENRICH in the edge case that the two can be swapped around.

…ich-after-lu-join

alex-spies · 2025-07-18T10:36:16Z

Thanks for the reviews @luigidellaquila and @smalyshev , very helpful feedback!

…ich-after-lu-join

elasticsearchmachine · 2025-07-18T13:32:36Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.1	Commit could not be cherrypicked due to conflicts
❌	9.0	Commit could not be cherrypicked due to conflicts
❌	8.18	Commit could not be cherrypicked due to conflicts

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

Fix elastic#129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](elastic#129372 (comment)) and my draft elastic#131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

Fix elastic#129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](elastic#129372 (comment)) and my draft elastic#131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

Fix elastic#129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](elastic#129372 (comment)) and my draft elastic#131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

alex-spies · 2025-07-18T14:55:32Z

💚 All backports created successfully

Status	Branch	Result
✅	9.1
✅	9.0
✅	8.19
✅	8.18

Questions ?

Please refer to the Backport tool documentation

Fix elastic#129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](elastic#129372 (comment)) and my draft elastic#131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

* ESQL: Disallow remote enrich after lu join (#131426) Fix #129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](#129372 (comment)) and my draft #131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>

Fix #129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](#129372 (comment)) and my draft #131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java

* ESQL: Disallow remote enrich after lu join (#131426) Fix #129372 Due to how remote ENRICH is [planned](https://github.com/elastic/elasticsearch/blob/32e50d0d94e27ee559d24bf9d5463ba6e64d1788/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java#L93), it interacts in special ways with pipeline breakers, in particular LIMIT and TopN; when these are encountered upstream from a remote ENRICH, these nodes are copied and executed a second time after the remote ENRICH. We'd like to allow remote ENRICH after LOOKUP JOIN, but that forces the lookup to be remote as well; this has its own interactions with pipeline breakers: in particular, LIMITs and TopNs cannot just be duplicated after LOOKUP JOIN, as LOOKUP JOIN may add new rows. For now, let's just forbid any usage of remote ENRICH after LOOKUP JOINs; remote ENRICH is mostly relevant for CCS, and LOOKUP JOIN doesn't support that in 9.1/8.19, anyway. There is separate work that enables remote LOOKUP JOINs on remote clusters and adds the correct validations; we can later build support for remote ENRICH + LOOKUP JOIN on top of that. (C.f. my comment [here](#129372 (comment)) and my draft #131286 for enabling this.) (cherry picked from commit 06e39c0) # Conflicts: # x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java # x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java * [CI] Auto commit changes from spotless * Add test_lookup index to default test analyzer * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>

alex-spies added 2 commits July 17, 2025 11:09

Disallow LOOKUP JOIN before remote ENRICH

7dd0088

Add some csv tests

ac18cfb

alex-spies requested review from smalyshev and bpintea July 17, 2025 09:55

alex-spies added auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.1.0 v9.2.0 v9.0.5 v8.18.5 labels Jul 17, 2025

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

alex-spies added the >bug label Jul 17, 2025

Update docs/changelog/131426.yaml

eb03085

alex-spies added 3 commits July 17, 2025 15:31

Make verification work with multiple FORK branches

11d3eb3

This was broken before: a coordinator-only ENRICH in one branch and a remote-only ENRICH in the other could be (wrongly) considered invalid depending on the order of the fork branches.

Exclude new tests from running in CCS

81463ac

This interprets LOOKUP JOIN as a remote-only LOOKUP JOIN which is more restricted.

Add comment

444b8f3

luigidellaquila approved these changes Jul 17, 2025

View reviewed changes

smalyshev approved these changes Jul 17, 2025

View reviewed changes

alex-spies added 5 commits July 18, 2025 12:03

Add tests: remote ENRICH + STATS/coord ENRICH

c0e72e1

More ENRICH csv tests for remote/coord mode

ade4ffe

Also use non-overwriting LU JOIN in VerifierTests

ca65578

Randomly use a lookup index that doesn't shadow the columns added by the ENRICH. This could be relevant if we at some point allow LU JOIN after remote ENRICH in the edge case that the two can be swapped around.

Merge remote-tracking branch 'upstream/main' into disallow-remote-enr…

aeebd76

…ich-after-lu-join

Make lambda more readable

7ac0248

alex-spies added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 18, 2025

Fix the added randomized verifier tests

47dca64

Merge remote-tracking branch 'upstream/main' into disallow-remote-enr…

19e77f8

…ich-after-lu-join

elasticsearchmachine merged commit 06e39c0 into elastic:main Jul 18, 2025
33 checks passed

alex-spies deleted the disallow-remote-enrich-after-lu-join branch July 18, 2025 13:31

elasticsearchmachine added the backport pending label Jul 18, 2025

alex-spies mentioned this pull request Jul 18, 2025

[9.1] ESQL: Disallow remote enrich after lu join (#131426) #131533

Merged

alex-spies mentioned this pull request Jul 18, 2025

[9.0] ESQL: Disallow remote enrich after lu join (#131426) #131534

Merged

alex-spies mentioned this pull request Jul 18, 2025

[8.19] ESQL: Disallow remote enrich after lu join (#131426) #131535

Merged

alex-spies mentioned this pull request Jul 18, 2025

[8.18] ESQL: Disallow remote enrich after lu join (#131426) #131537

Merged

smalyshev removed the backport pending label Jul 25, 2025

ESQL: Disallow remote enrich after lu join #131426

ESQL: Disallow remote enrich after lu join #131426

Uh oh!

Conversation

alex-spies commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 17, 2025

Uh oh!

elasticsearchmachine commented Jul 17, 2025

Uh oh!

luigidellaquila 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smalyshev commented Jul 17, 2025

Uh oh!

alex-spies commented Jul 18, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 18, 2025

💔 Backport failed

Uh oh!

alex-spies commented Jul 18, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Uh oh!

alex-spies commented Jul 17, 2025 •

edited

Loading