Support enrich ANY mode in cross clusters query #104840

dnhatn · 2024-01-27T23:54:38Z

Note to reviewers: I apologize for the size of this PR, but I have tried to minimize its scope. You can skip most of the changes except for the EnrichPolicyResolver class, which contains the main changes.

This PR enables the resolution of enrich policies from multiple clusters, involving the following steps:

Calculate the policies that need to resolve for each cluster.
Send out LookupRequest to each cluster to resolve policies. Internally, a remote cluster handles the lookup in two steps:
- Ensure the caller has permission to access the enrich policies (security tests will be added later).
- For each found enrich policy, use IndexResolver to resolve the mappings of the concrete enrich index.
For each unresolved policy, combines the lookup results to compute the actual enrich policy and mappings depending on the enrich mode.

I considered alternative approaches, but this approach requires at most one cross-cluster call for each cluster.

With this capability, the ENRICH mode ANY now works across clusters. I have enabled enrich in MultiClusterSpecIT and added small IT tests. Additional tests will be added in subsequent PRs.

I plan to have several follow-ups to fully support enrich in cross-cluster queries.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java

elasticsearchmachine · 2024-01-29T02:19:35Z

Hi @dnhatn, I've created a changelog YAML for you.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

.../src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersEnrichIT.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java

elasticsearchmachine · 2024-01-29T04:35:22Z

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

astefan

Looking good. Left some minor comments.

...ugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/CsvTestsDataLoader.java

.../src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersEnrichIT.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java

...plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolverTests.java

dnhatn · 2024-01-29T17:07:17Z

@astefan Thanks for the detailed review. It's ready again.

astefan

LGTM
Left few very small comments.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java

costin

Minor comments from my side - thanks for the large suite of tests!

costin · 2024-01-30T02:06:54Z

...in/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEnrichTestCase.java

@@ -148,7 +148,7 @@ public void testNonExistentEnrichPolicy() throws IOException {
        );
        assertThat(
            EntityUtils.toString(re.getResponse().getEntity()),
-            containsString("unresolved enrich policy [countris], did you mean [countries]?")
+            containsString("enrich policy [countris] doesn't exist, did you mean [countries]?")


As there might a security setting (thus the policy can exist but not be visible) might be better to have the "cannot find enrich policy [countries], did you mean [...]).

It can happen in the future, but the current security model is all or nothing for enrich policies. I will look into it later, as I am working on the coordinator mode based on this PR.

costin · 2024-01-30T02:11:51Z

.../src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersEnrichIT.java

+    @Before
+    public void setupHostsEnrich() {
+        // the hosts policy are identical on every node
+        Map<String, String> allHosts = Map.of(


Not sure if there's value in externalizing the content to a .properties file and which can simply read afterwards.

costin · 2024-01-30T02:27:47Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/PreAnalyzer.java

@@ -23,11 +23,11 @@ public static class PreAnalysis {
        public static final PreAnalysis EMPTY = new PreAnalysis(emptyList(), emptyList());

        public final List<TableInfo> indices;
-        public final List<String> policyNames;
+        public final List<Enrich> unresolvedEnriches;


Everything is unresolved in the preAnalyzer - I suggest using policies or enrichPolicies instead or enriches.

++, I have renamed this to enriches in 11d4d7e

costin · 2024-01-30T02:30:05Z

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

+        if (PlannerUtils.hasUnsupportedEnrich(physicalPlan)) {
+            listener.onFailure(new IllegalArgumentException("Enrich modes COORDINATOR and REMOTE are not supported yet"));


This is better handled in the Verifier - no need to hold the PR for it though.

This will be removed shortly.

dnhatn · 2024-01-30T04:39:29Z

@astefan @nik9000 @costin Thanks for reviews.

elasticsearchmachine added the v8.13.0 label Jan 27, 2024

dnhatn force-pushed the enrich-any-mode branch 3 times, most recently from 909753d to bbf39a7 Compare January 28, 2024 07:44

Support enrich ANY mode in cross clusters query

8bbedbf

dnhatn force-pushed the enrich-any-mode branch from bbf39a7 to 8bbedbf Compare January 28, 2024 18:32

dnhatn commented Jan 29, 2024

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java Outdated Show resolved Hide resolved

dnhatn added :Analytics/ES|QL AKA ESQL >enhancement labels Jan 29, 2024

dnhatn requested review from nik9000, astefan and costin January 29, 2024 02:19

Update docs/changelog/104840.yaml

ad8d2c9

dnhatn commented Jan 29, 2024

View reviewed changes

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichPolicyResolver.java Show resolved Hide resolved

dnhatn marked this pull request as ready for review January 29, 2024 04:34

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 29, 2024

astefan reviewed Jan 29, 2024

View reviewed changes

dnhatn added 4 commits January 29, 2024 08:57

static list

a8194ac

detailed error message

c8bc40e

relax mapping check

efb7fe0

More restrict enrich fields

36e9917

dnhatn requested a review from astefan January 29, 2024 17:06

dnhatn added 2 commits January 29, 2024 12:32

wording

c68cae8

spotless

6bb9247

astefan approved these changes Jan 29, 2024

View reviewed changes

more wording

8055d9b

dnhatn added 5 commits January 29, 2024 14:45

spelling

96cee5e

comment

4eaed4f

suggestion

e731bab

more wording

307a130

Merge branch 'main' into enrich-any-mode

b53d1c2

costin approved these changes Jan 30, 2024

View reviewed changes

naming

11d4d7e

dnhatn merged commit 9c086de into elastic:main Jan 30, 2024

dnhatn deleted the enrich-any-mode branch January 30, 2024 04:41

This was referenced Jan 30, 2024

Add security tests for cross clusters query #104879

Merged

Adjust enrich policy not found error message #104945

Merged

dnhatn mentioned this pull request Feb 14, 2024

ESQL: Introduce Cross Cluster Query (CCQ) support #102954

Open

7 tasks

		if (PlannerUtils.hasUnsupportedEnrich(physicalPlan)) {
		listener.onFailure(new IllegalArgumentException("Enrich modes COORDINATOR and REMOTE are not supported yet"));

Support enrich ANY mode in cross clusters query #104840

Support enrich ANY mode in cross clusters query #104840

Uh oh!

Conversation

dnhatn commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 29, 2024

Uh oh!

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

dnhatn commented Jan 29, 2024

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

costin Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

dnhatn Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

costin Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

costin Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

dnhatn Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

costin Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

dnhatn Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

dnhatn commented Jan 30, 2024

Uh oh!

Uh oh!

dnhatn commented Jan 27, 2024 •

edited

Loading