-
Notifications
You must be signed in to change notification settings - Fork 25.8k
ESQL: Forbid "load" unmapped_fields for certain commands #144115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6fec64d
6cd153c
6880aa5
739d67a
840d414
97e63de
fa961b6
5ce6f9b
8c4c8f7
330726c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
|
|
||
| package org.elasticsearch.xpack.esql.analysis; | ||
|
|
||
| import org.elasticsearch.index.IndexMode; | ||
| import org.elasticsearch.license.XPackLicenseState; | ||
| import org.elasticsearch.xpack.esql.LicenseAware; | ||
| import org.elasticsearch.xpack.esql.capabilities.ConfigurationAware; | ||
|
|
@@ -31,13 +32,16 @@ | |
| import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.NotEquals; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Aggregate; | ||
| import org.elasticsearch.xpack.esql.plan.logical.EsRelation; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Fork; | ||
| import org.elasticsearch.xpack.esql.plan.logical.InlineStats; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Insist; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Limit; | ||
| import org.elasticsearch.xpack.esql.plan.logical.LimitBy; | ||
| import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Lookup; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Project; | ||
| import org.elasticsearch.xpack.esql.plan.logical.Subquery; | ||
| import org.elasticsearch.xpack.esql.plan.logical.UnionAll; | ||
| import org.elasticsearch.xpack.esql.plan.logical.promql.PromqlCommand; | ||
| import org.elasticsearch.xpack.esql.telemetry.FeatureMetric; | ||
| import org.elasticsearch.xpack.esql.telemetry.Metrics; | ||
|
|
@@ -86,9 +90,10 @@ public Verifier(Metrics metrics, XPackLicenseState licenseState, List<BiConsumer | |
| * | ||
| * @param plan The logical plan to be verified | ||
| * @param partialMetrics a bitset indicating a certain command (or "telemetry feature") is present in the query | ||
| * @param unmappedResolution the active unmapped-field resolution strategy; used to gate commands unsupported in certain modes | ||
| * @return a collection of verification failures; empty if and only if the plan is valid | ||
| */ | ||
| Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics) { | ||
| Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics, UnmappedResolution unmappedResolution) { | ||
| assert partialMetrics != null; | ||
| Failures failures = new Failures(); | ||
|
|
||
|
|
@@ -102,6 +107,10 @@ Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics) { | |
| return failures.failures(); | ||
| } | ||
|
|
||
| if (unmappedResolution == UnmappedResolution.LOAD) { | ||
| checkLoadModeDisallowedCommands(plan, failures); | ||
| } | ||
|
|
||
| // collect plan checkers | ||
| var planCheckers = planCheckers(plan); | ||
| planCheckers.addAll(extraCheckers); | ||
|
|
@@ -340,6 +349,24 @@ private static void checkLimitBy(LogicalPlan plan, Failures failures) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@code unmapped_fields="load"} does not yet support branching commands (FORK, LOOKUP JOIN, subqueries/views). | ||
| * See https://github.com/elastic/elasticsearch/issues/142033 | ||
| */ | ||
| private static void checkLoadModeDisallowedCommands(LogicalPlan plan, Failures failures) { | ||
| plan.forEachDown(p -> { | ||
| if (p instanceof Fork && p instanceof UnionAll == false) { | ||
| failures.add(fail(p, "FORK is not supported with unmapped_fields=\"load\"")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for actual subqueries, we'll get the |
||
| } | ||
| if (p instanceof Subquery) { | ||
| failures.add(fail(p, "Subqueries and views are not supported with unmapped_fields=\"load\"")); | ||
| } | ||
| if (p instanceof EsRelation esRelation && esRelation.indexMode() == IndexMode.LOOKUP) { | ||
| failures.add(fail(p, "LOOKUP JOIN is not supported with unmapped_fields=\"load\"")); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private void licenseCheck(LogicalPlan plan, Failures failures) { | ||
| Consumer<Node<?>> licenseCheck = n -> { | ||
| if (n instanceof LicenseAware la && la.licenseCheck(licenseState) == false) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,6 @@ | |
|
|
||
| package org.elasticsearch.xpack.esql.analysis; | ||
|
|
||
| import org.elasticsearch.index.IndexMode; | ||
| import org.elasticsearch.test.ESTestCase; | ||
| import org.elasticsearch.xpack.esql.VerificationException; | ||
| import org.elasticsearch.xpack.esql.action.EsqlCapabilities; | ||
|
|
@@ -2471,11 +2470,11 @@ public void testSubquerysMixAndLookupJoinNullify() { | |
| assertThat(Expressions.names(plan.output()), is(List.of("COUNT(*)", "empNo", "languageCode", "does_not_exist2"))); | ||
| } | ||
|
|
||
| // same tree as above, except for the source nodes | ||
| // unmapped_fields="load" disallows subqueries and LOOKUP JOIN (see #142033) | ||
| public void testSubquerysMixAndLookupJoinLoad() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| var plan = analyzeStatement(setUnmappedLoad(""" | ||
| var e = expectThrows(VerificationException.class, () -> analyzeStatement(setUnmappedLoad(""" | ||
| FROM test, | ||
| (FROM languages | ||
| | WHERE language_code > 10 | ||
|
|
@@ -2489,36 +2488,12 @@ public void testSubquerysMixAndLookupJoinLoad() { | |
| | STATS COUNT(*) BY emp_no, language_code, does_not_exist2 | ||
| | RENAME emp_no AS empNo, language_code AS languageCode | ||
| | MV_EXPAND languageCode | ||
| """)); | ||
|
|
||
| // TODO: golden testing | ||
| assertThat(Expressions.names(plan.output()), is(List.of("COUNT(*)", "empNo", "languageCode", "does_not_exist2"))); | ||
|
|
||
| List<EsRelation> esRelations = plan.collect(EsRelation.class); | ||
| assertThat( | ||
| esRelations.stream().map(EsRelation::indexPattern).toList(), | ||
| is( | ||
| List.of( | ||
| "test", // FROM | ||
| "languages", | ||
| "sample_data", | ||
| "test", // LOOKUP JOIN | ||
| "languages_lookup" | ||
| ) | ||
| ) | ||
| ); | ||
| for (var esr : esRelations) { | ||
| if (esr.indexMode() != IndexMode.LOOKUP) { | ||
| var dne = esr.output().stream().filter(a -> a.name().startsWith("does_not_exist")).toList(); | ||
| assertThat(dne.size(), is(2)); | ||
| var dne1 = as(dne.getFirst(), FieldAttribute.class); | ||
| var dne2 = as(dne.getLast(), FieldAttribute.class); | ||
| var pukesf1 = as(dne1.field(), PotentiallyUnmappedKeywordEsField.class); | ||
| var pukesf2 = as(dne2.field(), PotentiallyUnmappedKeywordEsField.class); | ||
| assertThat(pukesf1.getName(), is("does_not_exist1")); | ||
| assertThat(pukesf2.getName(), is("does_not_exist2")); | ||
| } | ||
| } | ||
| """))); | ||
| String msg = e.getMessage(); | ||
| assertThat(msg, containsString("Found 4 problems")); | ||
| assertThat(msg, containsString("Subqueries and views are not supported with unmapped_fields=\"load\"")); | ||
| assertThat(msg, containsString("LOOKUP JOIN is not supported with unmapped_fields=\"load\"")); | ||
| assertThat(msg, not(containsString("FORK is not supported"))); | ||
| } | ||
|
|
||
| public void testFailSubquerysWithNoMainAndStatsOnlyNullify() { | ||
|
|
@@ -3795,6 +3770,163 @@ public void testStatsFilteredAggAfterEvalWithDottedUnmappedFieldFromIndex() { | |
| } | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsFork() { | ||
| verificationFailure( | ||
| setUnmappedLoad("FROM test | FORK (WHERE emp_no > 1) (WHERE emp_no < 100)"), | ||
| "FORK is not supported with unmapped_fields=\"load\"" | ||
| ); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsForkWithStats() { | ||
| verificationFailure( | ||
| setUnmappedLoad("FROM test | FORK (STATS c = COUNT(*)) (STATS d = AVG(salary))"), | ||
| "FORK is not supported with unmapped_fields=\"load\"" | ||
| ); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsForkWithMultipleBranches() { | ||
| verificationFailure(setUnmappedLoad(""" | ||
| FROM test | ||
| | FORK (WHERE emp_no > 1) | ||
| (WHERE emp_no < 100) | ||
| (WHERE salary > 50000) | ||
| """), "FORK is not supported with unmapped_fields=\"load\""); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsLookupJoin() { | ||
| verificationFailure( | ||
| setUnmappedLoad("FROM test | EVAL language_code = languages | LOOKUP JOIN languages_lookup ON language_code"), | ||
| "LOOKUP JOIN is not supported with unmapped_fields=\"load\"" | ||
| ); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsLookupJoinAfterFilter() { | ||
| verificationFailure(setUnmappedLoad(""" | ||
| FROM test | ||
| | WHERE emp_no > 1 | ||
| | EVAL language_code = languages | ||
| | LOOKUP JOIN languages_lookup ON language_code | ||
| | KEEP emp_no, language_name | ||
| """), "LOOKUP JOIN is not supported with unmapped_fields=\"load\""); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsSubquery() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| verificationFailure( | ||
| setUnmappedLoad("FROM test, (FROM languages | WHERE language_code > 1)"), | ||
| "Subqueries and views are not supported with unmapped_fields=\"load\"" | ||
| ); | ||
| } | ||
|
|
||
| public void testLoadModeAllowsSingleSubquery() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| // A single subquery without a main index is merged into the main query during analysis, | ||
| // so there is no Subquery node in the plan and no branching — this is allowed. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++, that's correct! |
||
| var plan = analyzeStatement(setUnmappedLoad("FROM (FROM languages | WHERE language_code > 1)")); | ||
|
|
||
| // TODO: golden testing | ||
| var limit = as(plan, Limit.class); | ||
| var filter = as(limit.child(), Filter.class); | ||
| var relation = as(filter.child(), EsRelation.class); | ||
| assertThat(relation.indexPattern(), is("languages")); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsMultipleSubqueries() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| verificationFailure(setUnmappedLoad(""" | ||
| FROM test, | ||
| (FROM languages | WHERE language_code > 1), | ||
| (FROM sample_data | STATS max(@timestamp)) | ||
| """), "Subqueries and views are not supported with unmapped_fields=\"load\""); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsNestedSubqueries() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| verificationFailure( | ||
| setUnmappedLoad("FROM test, (FROM languages, (FROM sample_data | STATS count(*)) | WHERE language_code > 10)"), | ||
| "Subqueries and views are not supported with unmapped_fields=\"load\"" | ||
| ); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsSubqueryWithLookupJoin() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| verificationFailure(setUnmappedLoad(""" | ||
| FROM test, | ||
| (FROM test | ||
| | EVAL language_code = languages | ||
| | LOOKUP JOIN languages_lookup ON language_code) | ||
| """), "Subqueries and views are not supported with unmapped_fields=\"load\""); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsForkAndLookupJoin() { | ||
| var query = setUnmappedLoad(""" | ||
| FROM test | ||
| | EVAL language_code = languages | ||
| | LOOKUP JOIN languages_lookup ON language_code | ||
| | FORK (WHERE emp_no > 1) (WHERE emp_no < 100) | ||
|
Comment on lines
+3868
to
+3871
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The query is duplicated. Let's extract that into a String that is re-used. It's correct that we assert both failures, though. |
||
| """); | ||
| verificationFailure(query, "FORK is not supported with unmapped_fields=\"load\""); | ||
| verificationFailure(query, "LOOKUP JOIN is not supported with unmapped_fields=\"load\""); | ||
| } | ||
|
|
||
| public void testLoadModeDisallowsSubqueryAndFork() { | ||
| assumeTrue("Requires subquery in FROM command support", EsqlCapabilities.Cap.SUBQUERY_IN_FROM_COMMAND.isEnabled()); | ||
|
|
||
| var query = setUnmappedLoad(""" | ||
| FROM test, (FROM languages | WHERE language_code > 1) | ||
| | FORK (WHERE emp_no > 1) (WHERE emp_no < 100) | ||
| """); | ||
| verificationFailure(query, "Subqueries and views are not supported with unmapped_fields=\"load\""); | ||
| verificationFailure(query, "FORK is not supported with unmapped_fields=\"load\""); | ||
| } | ||
|
|
||
| public void testLoadModeAllowsInlineStats() { | ||
| var plan = analyzeStatement(setUnmappedLoad(""" | ||
| FROM test | ||
| | INLINE STATS c = COUNT(*) BY emp_no | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not-so nit: we already have |
||
| """)); | ||
|
|
||
| // TODO: golden testing | ||
| var limit = as(plan, Limit.class); | ||
| var inlineStats = as(limit.child(), InlineStats.class); | ||
| var agg = as(inlineStats.child(), Aggregate.class); | ||
| assertThat(Expressions.names(agg.groupings()), is(List.of("emp_no"))); | ||
| var relation = as(agg.child(), EsRelation.class); | ||
| assertThat(relation.indexPattern(), is("test")); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: asserting the plan structure generally makes sense if we already add a positive test. And/or leaving a comment |
||
|
|
||
| public void testLoadModeAllowsInlineStatsWithUnmappedFields() { | ||
| var plan = analyzeStatement(setUnmappedLoad(""" | ||
| FROM test | ||
| | INLINE STATS s = COUNT(does_not_exist1), c = COUNT(*) BY does_not_exist2, emp_no | ||
| """)); | ||
|
|
||
| // TODO: golden testing | ||
| var limit = as(plan, Limit.class); | ||
| var inlineStats = as(limit.child(), InlineStats.class); | ||
| var agg = as(inlineStats.child(), Aggregate.class); | ||
| assertThat(Expressions.names(agg.groupings()), is(List.of("does_not_exist2", "emp_no"))); | ||
| assertThat(Expressions.names(agg.aggregates()), is(List.of("s", "c", "does_not_exist2", "emp_no"))); | ||
| var relation = as(agg.child(), EsRelation.class); | ||
| assertThat(relation.indexPattern(), is("test")); | ||
| var dneAttrs = relation.output().stream().filter(a -> a.name().startsWith("does_not_exist")).toList(); | ||
| assertThat(dneAttrs, hasSize(2)); | ||
| } | ||
|
|
||
| public void testNullifyModeAllowsFork() { | ||
| var plan = analyzeStatement(setUnmappedNullify("FROM test | FORK (WHERE emp_no > 1) (WHERE emp_no < 100)")); | ||
|
|
||
| // TODO: golden testing | ||
| var limit = as(plan, Limit.class); | ||
| var fork = as(limit.child(), Fork.class); | ||
| assertThat(fork.children(), hasSize(2)); | ||
| } | ||
|
|
||
| private void verificationFailure(String statement, String expectedFailure) { | ||
| var e = expectThrows(VerificationException.class, () -> analyzeStatement(statement)); | ||
| assertThat(e.getMessage(), containsString(expectedFailure)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We could add a comment pointing to #142033?
Also applies elsewhere where this PR disables testing of some queries with load.