Skip to content

Remote Lookup Join implementation#129013

Merged
smalyshev merged 85 commits intoelastic:mainfrom
smalyshev:remote-lookup-join
Jul 10, 2025
Merged

Remote Lookup Join implementation#129013
smalyshev merged 85 commits intoelastic:mainfrom
smalyshev:remote-lookup-join

Conversation

@smalyshev
Copy link
Contributor

@smalyshev smalyshev commented Jun 5, 2025 •
edited
Loading

This patch enables using LOOKUP JOIN with cross-cluster queries. Example:

FROM logs-*, remote:logs-* | LOOKUP JOIN clients on ip | SORT timestamp | LIMIT 100

Currently some constructs are unsupported before LOOKUP JOIN when remote indices are involved:

  • SORT
  • LIMIT
  • STATS
  • ENRICH _coordinator:...
  • FORK

The LOOKUP JOIN with remote indices automatically assumes that the joins are executed remotely - i.e. LOOKUP JOIN with remote indices is forced to execute on the remote side of the query (inside FragmentExec) - this is the source of the most exclusions above.
@smalyshev smalyshev force-pushed the remote-lookup-join branch 3 times, most recently from cceb86d to bb3d64b Compare June 10, 2025 18:03
@smalyshev smalyshev force-pushed the remote-lookup-join branch from 29df38f to 59d98b2 Compare June 19, 2025 02:05
@smalyshev smalyshev force-pushed the remote-lookup-join branch from cb90a8b to d8ec53d Compare June 27, 2025 17:43
@elastic elastic deleted a comment from github-actions bot Jun 30, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jun 30, 2025

Hi @smalyshev, I've created a changelog YAML for you.
@smalyshev smalyshev changed the title WIP: Remote lookup join implementation Jul 1, 2025
@smalyshev smalyshev marked this pull request as ready for review July 1, 2025 00:01
@smalyshev smalyshev requested a review from alex-spies July 1, 2025 00:01
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Jul 1, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 1, 2025

Pinging @elastic/es-analytical-engine (Team:Analytics)
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 1, 2025

Pinging @elastic/es-search-foundations (Team:Search Foundations)
alex-spies
alex-spies reviewed Jul 8, 2025
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done now :) This is very nice!

I think this can nearly be merged, I would only make sure that we don't accidentally skip fwc tests as mentioned here. Other than that, my remarks are mostly on the minor side and also you already addressed a bunch of them :)

I mentioned some thoughts for follow-ups here. In addition, I saw many FIXMEs in the tests that I agree with. But that's for future work :)
...ulti-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java
Comment on lines +314 to +315
// If the query contains lookup indices, use only remotes to avoid duplication
onlyRemotes = true;
Copy link
Contributor

@alex-spies alex-spies Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err, doesn't this mean this suite doesn't at all test the case where a lookup happens both on remote and the local cluster?
Copy link
Contributor Author

@smalyshev smalyshev Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one doesn't because it wouldn't match the results - we'd have more rows than the local case. We have IT tests for that case. I haven't found any easy way to make these tests work in "both" case without substantially changing them.
...c/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterLookupJoinIT.java
ZonedDateTime nowUtc = ZonedDateTime.now(ZoneOffset.UTC);
ZonedDateTime nextMidnight = nowUtc.plusDays(1).withHour(0).withMinute(0).withSecond(0).withNano(0);
// If we're too close to midnight, we could create index with one day and query with another, and it'd fail.
assumeTrue("Skip if too close to midnight", Duration.between(nowUtc, nextMidnight).toMinutes() >= 5);
Copy link
Contributor

@alex-spies alex-spies Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
...c/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterLookupJoinIT.java
assertThat(columns, hasItems("lookup_key", "lookup_name", "lookup_tag", "v", "tag"));

List<List<Object>> values = getValuesList(resp);
assertThat(values, hasSize(10));
Copy link
Contributor

@alex-spies alex-spies Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I really don't know if a missing remote lookup index should lead to skipping the whole remote. That's a behavior we need to double check in the future.
alex-spies
alex-spies approved these changes Jul 9, 2025
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this looks mighty good already. Let's :shipit:

I'm not sure anymore if we already have a test that confirms that remote lookup joins remain disabled on non-SNAPSHOT builds. (This PR adds so many tests, I lost track. Nice test coverage, though!)

Could you please double check that we have such a test before merging? As a failsafe against accidentally enabling/releasing the feature prematurely.
@smalyshev
Copy link
Contributor Author

smalyshev commented Jul 9, 2025

I'm not sure anymore if we already have a test that confirms that remote lookup joins remain disabled on non-SNAPSHOT builds.

We don't have a specific test for that, how would I test that?
@alex-spies
Copy link
Contributor

alex-spies commented Jul 9, 2025

We don't have a specific test for that, how would I test that?

Hm, we could add an IT with assumeFalse(Build.current().isSnapshot()). This doesn't really require CCS infrastructure as we're supposed to fail during parsing, so we could just add this to org.elasticsearch.xpack.esql.qa.single_node.RestEsqlIT, maybe? It should fail when attempting to run FROM remote:idx | LOOKUP JOIN ... (or any other similar query).
@smalyshev smalyshev enabled auto-merge (squash) July 9, 2025 22:58
@smalyshev smalyshev merged commit e4155ea into elastic:main Jul 10, 2025
32 of 33 checks passed
@alex-spies alex-spies mentioned this pull request Jul 15, 2025
Closed
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 17, 2025
@smalyshev @mridula-s109
Remote Lookup Join implementation (elastic#129013)
f34ca9a 
Remote lookup join implementation

This patch enables using LOOKUP JOIN with cross-cluster queries. Example:
FROM logs-*, remote:logs-* | LOOKUP JOIN clients on ip | SORT timestamp | LIMIT 100
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 17, 2025
@smalyshev @mridula-s109
Remote Lookup Join implementation (elastic#129013)
ebbdfd8 
Remote lookup join implementation

This patch enables using LOOKUP JOIN with cross-cluster queries. Example:
FROM logs-*, remote:logs-* | LOOKUP JOIN clients on ip | SORT timestamp | LIMIT 100
This was referenced Jul 18, 2025
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >feature release highlight :Search Foundations/CCS Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.2.0

5 participants

@smalyshev @elasticsearchmachine @alex-spies @idegtiarenko @tylerperk