ES|QL: Allow FORK with remote indices by ioanatia · Pull Request #128310 · elastic/elasticsearch

ioanatia · 2025-05-22T13:42:30Z

We initially skipped CCS tests (PR) after we merged streaming support for FORK because the tests were flaky. In theory, given the FORK's execution model, queries using FORK with remote indices should just work.

This was back in May and since then CCS has stabilized for the GA release and we no longer see these failures.
I am not sure whether there was a single fix that enabled us to enable querying remote indices with FORK.
There are probably more separate changes (#127328, #127328 etc) that compounded to this moment where we can support FORK with CCS.

elasticsearchmachine · 2025-05-22T14:58:58Z

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

elasticsearchmachine · 2025-05-22T14:58:58Z

Pinging @elastic/es-search-relevance (Team:Search Relevance)

ioanatia · 2025-05-22T19:45:38Z

got another failure after merging main - cannot replicate locally 😞

seems to be the same type of issue as before with results not coming back:

java.lang.AssertionError: Expected more data but no more entries found after [3] |  
-- | --
  | Data mismatch: |  
  |   |  
  | Actual: |  
  | _fork:keyword \| emp_no:integer \| x:keyword \| y:keyword \| z:keyword \| a:keyword |  
  | fork1         \| 10048          \| Florian   \| 10048     \| Syrotiuk  \| Florian 10048 Syrotiuk |  
  | fork1         \| 10081          \| Zhongwei  \| 10081     \| Rosen     \| Zhongwei 10081 Rosen |  
  | fork2         \| null           \| 2         \| 10081     \| 10048     \| null |  
  |   |  
  | Expected: |  
  | _fork:keyword \| emp_no:integer \| x:keyword \| y:keyword \| z:keyword \| a:keyword |  
  | fork1         \| 10048          \| Florian   \| 10048     \| Syrotiuk  \| Florian 10048 Syrotiuk |  
  | fork1         \| 10081          \| Zhongwei  \| 10081     \| Rosen     \| Zhongwei 10081 Rosen |  
  | fork2         \| null           \| 2         \| 10081     \| 10048     \| null |  
  | fork3         \| 10048          \| Syrotiuk  \| null      \| null      \| null |  
  | fork3         \| 10081          \| Rosen     \| null      \| null      \| null |  
  | fork4         \| 10048          \| abc       \| aaa       \| null      \| null |  
  | fork4         \| 10081          \| abc       \| aaa       \| null      \| null |  
  |

putting this back in draft until I figure it out

idegtiarenko

👍
Looking forward for this stabilized and merged

ioanatia · 2025-09-02T14:12:07Z

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

-| FORK ( WHERE language_name == "English" | EVAL x = 1 )
-       ( WHERE language_name != "English" )
-| SORT _fork, language_name
+FROM employees


all the tests that were querying the languages index failed - with rows being duplicated:

Actual: | -- | -- language_name:keyword \| x:integer \| _fork:keyword | English \| 1 \| fork1 | English \| 1 \| fork1 | French \| null \| fork2 | French \| null \| fork2 | German \| null \| fork2 | German \| null \| fork2 | Spanish \| null \| fork2 | Spanish \| null \| fork2 | | Expected: | language_name:keyword \| x:integer \| _fork:keyword | English \| 1 \| fork1 | French \| null \| fork2 | German \| null \| fork2 | Spanish \| null \| fork2

debugging these test showed that we were querying both the local and remote index, which is why we had duplicate results.

languages was an index that was primarily used for testing lookup join for which we had some special handling such that we disallow duplicates:

elasticsearch/x-pack/plugin/esql/qa/server/multi-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java

Lines 298 to 302 in b1081c0

if (Arrays.stream(localIndices).anyMatch(i -> LOOKUP_INDICES.contains(i.trim().toLowerCase(Locale.ROOT)))) {

// If the query contains lookup indices, use only remotes to avoid duplication

onlyRemotes = true;

}

final boolean onlyRemotesFinal = onlyRemotes;

languages is not part of LOOKUP_INDICES (but it should be?)
anyway, I thought that it might be better to rewrite these tests with an index that has no special handling - so I used employees here instead of languages.

ioanatia · 2025-09-02T14:46:28Z

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

        }

        return input -> {
-            checkForRemoteClusters(input, source(ctx), "FORK");


we could have made ENABLE_FORK_FOR_REMOTE_INDICES capability available only in snapshots and use it here - such that CCS support is only available in snapshots - but I don't think we need to 🤔

smalyshev · 2025-09-02T16:46:34Z

Is it still true that cross-cluster FORK requires coordinator-side aggregation? I'm not sure I understand the mechanism why.

ioanatia · 2025-09-02T18:09:20Z

Is it still true that cross-cluster FORK requires coordinator-side aggregation? I'm not sure I understand the mechanism why.

I am not sure what you mean by coordinator-side aggregation.
A query using FORK gets split into multiple plans - a subplan for each branch and a main coordinator plan.
Each subplan that pertains to a FORK branch gets split into a data node plan and coordinator plan.

For example:

FROM test
| FORK
    ( WHERE content:"fox" ) // sub plan 1
    ( WHERE content:"dog" ) // sub plan 2
| SORT _fork
| KEEP _fork, id, content

gets translated to something like this:

It was a conscious decision to execute FORK this way and one of the benefits was that we did not need to make any special concessions for CCS support.
Anyway, this PR does not make any fundamental changes in how FORK gets planned - we simply enable CCS support here.

ioanatia · 2025-09-02T18:30:04Z

Is it still true that cross-cluster FORK requires coordinator-side aggregation? I'm not sure I understand the mechanism why.

Checked with @smalyshev - this is about having FORK marked as ExecutesOn.Coordinator, similar to STATS:

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

Line 36 in ebb94bd

    
           public class Fork extends LogicalPlan implements PostAnalysisPlanVerificationAware, TelemetryAware, ExecutesOn.Coordinator {

We agreed that for the beginning this is fine, even though it puts some restrictions in place such as we cannot execute a remote lookup join after FORK.
We will look into removing this restriction in the future - e.g. if we can push more plans that come after FORK to the subplans etc.

leemthompo · 2025-09-03T08:15:07Z

@ioanatia I guess can update the docs to mention that the the limitation disappears in 9.2.0

elasticsearch/docs/reference/query-languages/esql/_snippets/commands/layout/fork.md

Line 42 in 208cd45

- Using remote cluster references and `FORK` is not supported.

dnhatn

Looks good. Thanks @ioanatia

ioanatia · 2025-09-03T18:15:49Z

@ioanatia I guess can update the docs to mention that the the limitation disappears in 9.2.0

I will get that addressed too. @leemthompo

Test FORK with multi cluster

dd28497

elasticsearchmachine added the v9.1.0 label May 22, 2025

ioanatia added :Analytics/ES|QL AKA ESQL :Search Relevance/Search Catch all for Search Relevance >non-issue labels May 22, 2025

ioanatia changed the title ~~ESTest FORK with multi cluster~~ ES|QL: Test FORK with multi cluster May 22, 2025

ioanatia marked this pull request as ready for review May 22, 2025 14:58

ioanatia requested review from ChrisHegarty and carlosdelest May 22, 2025 14:58

elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels May 22, 2025

ioanatia requested a review from tteofili May 22, 2025 14:58

ioanatia mentioned this pull request May 21, 2025

Initial FORK with restrictions #121950

Closed

23 tasks

Merge branch 'main' into fork_ccs_tests

01ba525

dnhatn approved these changes May 22, 2025

View reviewed changes

ioanatia marked this pull request as draft May 22, 2025 19:46

elasticsearchmachine added v9.2.0 and removed v9.1.0 labels Jun 26, 2025

ioanatia mentioned this pull request Jun 30, 2025

ES|QL Fork Command (GA) #121652

Closed

18 tasks

ioanatia and others added 2 commits August 29, 2025 19:59

Merge branch 'main' into fork_ccs_tests

f8e9654

[CI] Auto commit changes from spotless

0e22c19

idegtiarenko approved these changes Sep 1, 2025

View reviewed changes

ioanatia and others added 4 commits September 2, 2025 12:37

remove verification check for CCS

2104136

Use an index in csv that is not used for lookup join tests

89c7db7

Bwc tests fix

fc65c06

Merge branch 'main' into fork_ccs_tests

e459889

ioanatia changed the title ~~ES|QL: Test FORK with multi cluster~~ ES|QL: Allow FORK with remote indices Sep 2, 2025

ioanatia commented Sep 2, 2025

View reviewed changes

ioanatia marked this pull request as ready for review September 2, 2025 14:50

ioanatia requested review from dnhatn and idegtiarenko September 2, 2025 15:07

idegtiarenko approved these changes Sep 3, 2025

View reviewed changes

dnhatn approved these changes Sep 3, 2025

View reviewed changes

ioanatia merged commit 580f734 into elastic:main Sep 3, 2025
33 checks passed

ioanatia deleted the fork_ccs_tests branch September 3, 2025 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES|QL: Allow FORK with remote indices#128310

ES|QL: Allow FORK with remote indices#128310
ioanatia merged 8 commits intoelastic:mainfrom
ioanatia:fork_ccs_tests

ioanatia commented May 22, 2025 •

edited

Loading

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

ioanatia commented May 22, 2025

Uh oh!

idegtiarenko left a comment

Uh oh!

ioanatia Sep 2, 2025

Uh oh!

ioanatia Sep 2, 2025

Uh oh!

smalyshev commented Sep 2, 2025

Uh oh!

ioanatia commented Sep 2, 2025 •

edited

Loading

Uh oh!

ioanatia commented Sep 2, 2025

Uh oh!

leemthompo commented Sep 3, 2025 •

edited

Loading

Uh oh!

dnhatn left a comment

Uh oh!

Uh oh!

ioanatia commented Sep 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	if (Arrays.stream(localIndices).anyMatch(i -> LOOKUP_INDICES.contains(i.trim().toLowerCase(Locale.ROOT)))) {
	// If the query contains lookup indices, use only remotes to avoid duplication
	onlyRemotes = true;
	}
	final boolean onlyRemotesFinal = onlyRemotes;

Conversation

ioanatia commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

elasticsearchmachine commented May 22, 2025

Uh oh!

ioanatia commented May 22, 2025

Uh oh!

idegtiarenko left a comment

Choose a reason for hiding this comment

Uh oh!

ioanatia Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

ioanatia Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

smalyshev commented Sep 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

ioanatia commented Sep 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ioanatia commented Sep 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ioanatia commented May 22, 2025 •

edited

Loading

ioanatia commented Sep 2, 2025 •

edited

Loading

leemthompo commented Sep 3, 2025 •

edited

Loading