From 041def0c3926e96b2647254fc09f00588a82b20b Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Fri, 13 Dec 2024 12:54:02 +0100 Subject: [PATCH 1/2] ESQL: Dependency check for binary plans (#118326) (#118654) Make the dependency checker for query plans take into account binary plans and make sure that fields required from the left hand side are actually obtained from there (and analogously for the right). --- .../xpack/esql/index/EsIndex.java | 3 + .../xpack/esql/optimizer/LogicalVerifier.java | 3 +- .../esql/optimizer/PhysicalVerifier.java | 9 +-- .../rules/PlanConsistencyChecker.java | 62 +++++++++++++++---- .../xpack/esql/plan/logical/BinaryPlan.java | 5 ++ .../xpack/esql/plan/logical/join/Join.java | 11 ++++ .../esql/plan/physical/AggregateExec.java | 4 +- .../xpack/esql/plan/physical/BinaryExec.java | 5 ++ .../esql/plan/physical/HashJoinExec.java | 10 +++ .../esql/plan/physical/LookupJoinExec.java | 15 +++++ .../esql/analysis/AnalyzerTestUtils.java | 8 ++- .../optimizer/LogicalPlanOptimizerTests.java | 29 ++++++++- .../optimizer/PhysicalPlanOptimizerTests.java | 59 +++++++++++++++++- 13 files changed, 195 insertions(+), 28 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/index/EsIndex.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/index/EsIndex.java index ee51a6f391a65..d3fc9e15e2e04 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/index/EsIndex.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/index/EsIndex.java @@ -25,6 +25,9 @@ public class EsIndex implements Writeable { private final Map mapping; private final Map indexNameWithModes; + /** + * Intended for tests. Returns an index with an empty index mode map. + */ public EsIndex(String name, Map mapping) { this(name, mapping, Map.of()); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java index 5e91425296822..dce828dbf192d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java @@ -14,7 +14,6 @@ public final class LogicalVerifier { - private static final PlanConsistencyChecker DEPENDENCY_CHECK = new PlanConsistencyChecker<>(); public static final LogicalVerifier INSTANCE = new LogicalVerifier(); private LogicalVerifier() {} @@ -25,7 +24,7 @@ public Failures verify(LogicalPlan plan) { Failures dependencyFailures = new Failures(); plan.forEachUp(p -> { - DEPENDENCY_CHECK.checkPlan(p, dependencyFailures); + PlanConsistencyChecker.checkPlan(p, dependencyFailures); if (failures.hasFailures() == false) { p.forEachExpression(ex -> { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java index 9132cf87541bb..4ec90fc1ed50a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java @@ -13,7 +13,6 @@ import org.elasticsearch.xpack.esql.core.expression.Expressions; import org.elasticsearch.xpack.esql.optimizer.rules.PlanConsistencyChecker; import org.elasticsearch.xpack.esql.plan.logical.Enrich; -import org.elasticsearch.xpack.esql.plan.physical.AggregateExec; import org.elasticsearch.xpack.esql.plan.physical.EnrichExec; import org.elasticsearch.xpack.esql.plan.physical.FieldExtractExec; import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; @@ -28,7 +27,6 @@ public final class PhysicalVerifier { public static final PhysicalVerifier INSTANCE = new PhysicalVerifier(); - private static final PlanConsistencyChecker DEPENDENCY_CHECK = new PlanConsistencyChecker<>(); private PhysicalVerifier() {} @@ -44,11 +42,6 @@ public Collection verify(PhysicalPlan plan) { } plan.forEachDown(p -> { - if (p instanceof AggregateExec agg) { - var exclude = Expressions.references(agg.ordinalAttributes()); - DEPENDENCY_CHECK.checkPlan(p, exclude, depFailures); - return; - } if (p instanceof FieldExtractExec fieldExtractExec) { Attribute sourceAttribute = fieldExtractExec.sourceAttribute(); if (sourceAttribute == null) { @@ -62,7 +55,7 @@ public Collection verify(PhysicalPlan plan) { ); } } - DEPENDENCY_CHECK.checkPlan(p, depFailures); + PlanConsistencyChecker.checkPlan(p, depFailures); }); if (depFailures.hasFailures()) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/PlanConsistencyChecker.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/PlanConsistencyChecker.java index 5101e3f73bfdf..d5bd110e8df74 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/PlanConsistencyChecker.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/PlanConsistencyChecker.java @@ -12,31 +12,42 @@ import org.elasticsearch.xpack.esql.core.expression.AttributeSet; import org.elasticsearch.xpack.esql.core.expression.NameId; import org.elasticsearch.xpack.esql.plan.QueryPlan; +import org.elasticsearch.xpack.esql.plan.logical.BinaryPlan; +import org.elasticsearch.xpack.esql.plan.physical.BinaryExec; import java.util.HashSet; import java.util.Set; import static org.elasticsearch.xpack.esql.common.Failure.fail; -public class PlanConsistencyChecker

> { +public class PlanConsistencyChecker { /** * Check whether a single {@link QueryPlan} produces no duplicate attributes and its children provide all of its required * {@link QueryPlan#references() references}. Otherwise, add * {@link org.elasticsearch.xpack.esql.common.Failure Failure}s to the {@link Failures} object. */ - public void checkPlan(P p, Failures failures) { - checkPlan(p, AttributeSet.EMPTY, failures); - } - - public void checkPlan(P p, AttributeSet exclude, Failures failures) { - AttributeSet refs = p.references(); - AttributeSet input = p.inputSet(); - AttributeSet missing = refs.subtract(input).subtract(exclude); - // TODO: for Joins, we should probably check if the required fields from the left child are actually in the left child, not - // just any child (and analogously for the right child). - if (missing.isEmpty() == false) { - failures.add(fail(p, "Plan [{}] optimized incorrectly due to missing references {}", p.nodeString(), missing)); + public static void checkPlan(QueryPlan p, Failures failures) { + if (p instanceof BinaryPlan binaryPlan) { + checkMissingBinary( + p, + binaryPlan.leftReferences(), + binaryPlan.left().outputSet(), + binaryPlan.rightReferences(), + binaryPlan.right().outputSet(), + failures + ); + } else if (p instanceof BinaryExec binaryExec) { + checkMissingBinary( + p, + binaryExec.leftReferences(), + binaryExec.left().outputSet(), + binaryExec.rightReferences(), + binaryExec.right().outputSet(), + failures + ); + } else { + checkMissing(p, p.references(), p.inputSet(), "missing references", failures); } Set outputAttributeNames = new HashSet<>(); @@ -49,4 +60,29 @@ public void checkPlan(P p, AttributeSet exclude, Failures failures) { } } } + + private static void checkMissingBinary( + QueryPlan plan, + AttributeSet leftReferences, + AttributeSet leftInput, + AttributeSet rightReferences, + AttributeSet rightInput, + Failures failures + ) { + checkMissing(plan, leftReferences, leftInput, "missing references from left hand side", failures); + checkMissing(plan, rightReferences, rightInput, "missing references from right hand side", failures); + } + + private static void checkMissing( + QueryPlan plan, + AttributeSet references, + AttributeSet input, + String detailErrorMessage, + Failures failures + ) { + AttributeSet missing = references.subtract(input); + if (missing.isEmpty() == false) { + failures.add(fail(plan, "Plan [{}] optimized incorrectly due to {} {}", plan.nodeString(), detailErrorMessage, missing)); + } + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/BinaryPlan.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/BinaryPlan.java index 91cd7f7a15840..dbd22dd297f88 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/BinaryPlan.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/BinaryPlan.java @@ -6,6 +6,7 @@ */ package org.elasticsearch.xpack.esql.plan.logical; +import org.elasticsearch.xpack.esql.core.expression.AttributeSet; import org.elasticsearch.xpack.esql.core.tree.Source; import java.util.Arrays; @@ -30,6 +31,10 @@ public LogicalPlan right() { return right; } + public abstract AttributeSet leftReferences(); + + public abstract AttributeSet rightReferences(); + @Override public final BinaryPlan replaceChildren(List newChildren) { return replaceChildren(newChildren.get(0), newChildren.get(1)); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java index 6af29fb23b3bb..a2c159e506880 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.AttributeSet; +import org.elasticsearch.xpack.esql.core.expression.Expressions; import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.Source; @@ -97,6 +98,16 @@ public List output() { return lazyOutput; } + @Override + public AttributeSet leftReferences() { + return Expressions.references(config().leftFields()); + } + + @Override + public AttributeSet rightReferences() { + return Expressions.references(config().rightFields()); + } + public List rightOutputFields() { AttributeSet leftInputs = left().outputSet(); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/AggregateExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/AggregateExec.java index 35f45250ed270..3c2d49567813c 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/AggregateExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/AggregateExec.java @@ -184,7 +184,9 @@ public List output() { @Override protected AttributeSet computeReferences() { - return mode.isInputPartial() ? new AttributeSet(intermediateAttributes) : Aggregate.computeReferences(aggregates, groupings); + return mode.isInputPartial() + ? new AttributeSet(intermediateAttributes) + : Aggregate.computeReferences(aggregates, groupings).subtract(new AttributeSet(ordinalAttributes())); } /** Returns the attributes that can be loaded from ordinals -- no explicit extraction is needed */ diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/BinaryExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/BinaryExec.java index 6f200bad17a72..9a1b76205b595 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/BinaryExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/BinaryExec.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.esql.plan.physical; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.xpack.esql.core.expression.AttributeSet; import org.elasticsearch.xpack.esql.core.tree.Source; import java.io.IOException; @@ -40,6 +41,10 @@ public PhysicalPlan right() { return right; } + public abstract AttributeSet leftReferences(); + + public abstract AttributeSet rightReferences(); + @Override public void writeTo(StreamOutput out) throws IOException { Source.EMPTY.writeTo(out); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/HashJoinExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/HashJoinExec.java index 5ae3702993fcb..362c83bf76213 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/HashJoinExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/HashJoinExec.java @@ -119,6 +119,16 @@ protected AttributeSet computeReferences() { return Expressions.references(leftFields); } + @Override + public AttributeSet leftReferences() { + return Expressions.references(leftFields); + } + + @Override + public AttributeSet rightReferences() { + return Expressions.references(rightFields); + } + @Override public HashJoinExec replaceChildren(PhysicalPlan left, PhysicalPlan right) { return new HashJoinExec(source(), left, right, matchFields, leftFields, rightFields, output); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java index 26fd12447e664..2aff38993aa98 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/LookupJoinExec.java @@ -119,6 +119,21 @@ protected AttributeSet computeReferences() { return Expressions.references(leftFields); } + @Override + public AttributeSet leftReferences() { + return Expressions.references(leftFields); + } + + @Override + public AttributeSet rightReferences() { + // TODO: currently it's hard coded that we add all fields from the lookup index. But the output we "officially" get from the right + // hand side is inconsistent: + // - After logical optimization, there's a FragmentExec with an EsRelation on the right hand side with all the fields. + // - After local physical optimization, there's just an EsQueryExec here, with no fields other than _doc mentioned and we don't + // insert field extractions in the plan, either. + return AttributeSet.EMPTY; + } + @Override public LookupJoinExec replaceChildren(PhysicalPlan left, PhysicalPlan right) { return new LookupJoinExec(source(), left, right, leftFields, rightFields, addedFields); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java index 4e89a09db9ed4..5e79e40b7e938 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTestUtils.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.esql.analysis; +import org.elasticsearch.index.IndexMode; import org.elasticsearch.xpack.core.enrich.EnrichPolicy; import org.elasticsearch.xpack.esql.EsqlTestUtils; import org.elasticsearch.xpack.esql.enrich.ResolvedEnrichPolicy; @@ -104,6 +105,11 @@ public static LogicalPlan analyze(String query, String mapping, QueryParams para return analyzer.analyze(plan); } + public static IndexResolution loadMapping(String resource, String indexName, IndexMode indexMode) { + EsIndex test = new EsIndex(indexName, EsqlTestUtils.loadMapping(resource), Map.of(indexName, indexMode)); + return IndexResolution.valid(test); + } + public static IndexResolution loadMapping(String resource, String indexName) { EsIndex test = new EsIndex(indexName, EsqlTestUtils.loadMapping(resource)); return IndexResolution.valid(test); @@ -118,7 +124,7 @@ public static IndexResolution expandedDefaultIndexResolution() { } public static IndexResolution defaultLookupResolution() { - return loadMapping("mapping-languages.json", "languages_lookup"); + return loadMapping("mapping-languages.json", "languages_lookup", IndexMode.LOOKUP); } public static EnrichResolution defaultEnrichResolution() { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index cd3e94b70b37a..fd2f4333cc5f4 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -149,6 +149,7 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning; import static org.elasticsearch.xpack.esql.analysis.Analyzer.NO_FIELDS; import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.analyze; +import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.defaultLookupResolution; import static org.elasticsearch.xpack.esql.core.expression.Literal.NULL; import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY; import static org.elasticsearch.xpack.esql.core.type.DataType.DOUBLE; @@ -221,7 +222,13 @@ public static void init() { EsIndex test = new EsIndex("test", mapping, Map.of("test", IndexMode.STANDARD)); IndexResolution getIndexResult = IndexResolution.valid(test); analyzer = new Analyzer( - new AnalyzerContext(EsqlTestUtils.TEST_CFG, new EsqlFunctionRegistry(), getIndexResult, enrichResolution), + new AnalyzerContext( + EsqlTestUtils.TEST_CFG, + new EsqlFunctionRegistry(), + getIndexResult, + defaultLookupResolution(), + enrichResolution + ), TEST_VERIFIER ); @@ -4896,6 +4903,26 @@ public void testPlanSanityCheck() throws Exception { assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references [salary")); } + public void testPlanSanityCheckWithBinaryPlans() throws Exception { + var plan = optimizedPlan(""" + FROM test + | RENAME languages AS language_code + | LOOKUP JOIN languages_lookup ON language_code + """); + + var project = as(plan, Project.class); + var limit = as(project.child(), Limit.class); + var join = as(limit.child(), Join.class); + + var joinWithInvalidLeftPlan = join.replaceChildren(join.right(), join.right()); + IllegalStateException e = expectThrows(IllegalStateException.class, () -> logicalOptimizer.optimize(joinWithInvalidLeftPlan)); + assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from left hand side [language_code")); + + var joinWithInvalidRightPlan = join.replaceChildren(join.left(), join.left()); + e = expectThrows(IllegalStateException.class, () -> logicalOptimizer.optimize(joinWithInvalidRightPlan)); + assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from right hand side [language_code")); + } + // https://github.com/elastic/elasticsearch/issues/104995 public void testNoWrongIsNotNullPruning() { var plan = optimizedPlan(""" diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java index ccb24397ddec2..d43e41aed6a0e 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java @@ -115,6 +115,7 @@ import org.elasticsearch.xpack.esql.plan.physical.HashJoinExec; import org.elasticsearch.xpack.esql.plan.physical.LimitExec; import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec; +import org.elasticsearch.xpack.esql.plan.physical.LookupJoinExec; import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.esql.plan.physical.ProjectExec; import org.elasticsearch.xpack.esql.plan.physical.TopNExec; @@ -155,6 +156,7 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning; import static org.elasticsearch.xpack.esql.SerializationTestUtils.assertSerialization; import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.analyze; +import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.defaultLookupResolution; import static org.elasticsearch.xpack.esql.core.expression.Expressions.name; import static org.elasticsearch.xpack.esql.core.expression.Expressions.names; import static org.elasticsearch.xpack.esql.core.expression.function.scalar.FunctionTestUtils.l; @@ -281,16 +283,30 @@ TestDataSource makeTestDataSource( String indexName, String mappingFileName, EsqlFunctionRegistry functionRegistry, + IndexResolution lookupResolution, EnrichResolution enrichResolution, SearchStats stats ) { Map mapping = loadMapping(mappingFileName); EsIndex index = new EsIndex(indexName, mapping, Map.of("test", IndexMode.STANDARD)); IndexResolution getIndexResult = IndexResolution.valid(index); - Analyzer analyzer = new Analyzer(new AnalyzerContext(config, functionRegistry, getIndexResult, enrichResolution), TEST_VERIFIER); + Analyzer analyzer = new Analyzer( + new AnalyzerContext(config, functionRegistry, getIndexResult, lookupResolution, enrichResolution), + TEST_VERIFIER + ); return new TestDataSource(mapping, index, analyzer, stats); } + TestDataSource makeTestDataSource( + String indexName, + String mappingFileName, + EsqlFunctionRegistry functionRegistry, + EnrichResolution enrichResolution, + SearchStats stats + ) { + return makeTestDataSource(indexName, mappingFileName, functionRegistry, defaultLookupResolution(), enrichResolution, stats); + } + TestDataSource makeTestDataSource( String indexName, String mappingFileName, @@ -2312,6 +2328,39 @@ public void testVerifierOnMissingReferences() { assertThat(e.getMessage(), containsString(" > 10[INTEGER]]] optimized incorrectly due to missing references [emp_no{f}#")); } + public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception { + // Do not assert serialization: + // This will have a LookupJoinExec, which is not serializable because it doesn't leave the coordinator. + var plan = physicalPlan(""" + FROM test + | RENAME languages AS language_code + | SORT language_code + | LOOKUP JOIN languages_lookup ON language_code + """, testData, false); + + var planWithInvalidJoinLeftSide = plan.transformUp(LookupJoinExec.class, join -> join.replaceChildren(join.right(), join.right())); + + var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinLeftSide)); + assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from left hand side [language_code")); + + var planWithInvalidJoinRightSide = plan.transformUp( + LookupJoinExec.class, + // LookupJoinExec.rightReferences() is currently EMPTY (hack); use a HashJoinExec instead. + join -> new HashJoinExec( + join.source(), + join.left(), + join.left(), + join.leftFields(), + join.leftFields(), + join.rightFields(), + join.output() + ) + ); + + e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinRightSide)); + assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from right hand side [language_code")); + } + public void testVerifierOnDuplicateOutputAttributes() { var plan = physicalPlan(""" from test @@ -6766,11 +6815,17 @@ private PhysicalPlan physicalPlan(String query) { } private PhysicalPlan physicalPlan(String query, TestDataSource dataSource) { + return physicalPlan(query, dataSource, true); + } + + private PhysicalPlan physicalPlan(String query, TestDataSource dataSource, boolean assertSerialization) { var logical = logicalOptimizer.optimize(dataSource.analyzer.analyze(parser.createStatement(query))); // System.out.println("Logical\n" + logical); var physical = mapper.map(logical); // System.out.println(physical); - assertSerialization(physical); + if (assertSerialization) { + assertSerialization(physical); + } return physical; } From 9e2d44b1ffffab218d783a4ebc8c9c3ecd4781d2 Mon Sep 17 00:00:00 2001 From: Tommaso Teofili Date: Fri, 13 Dec 2024 14:22:53 +0100 Subject: [PATCH 2/2] `_score` should not be a reserved attribute in ES|QL (#118435) (#118568) (cherry picked from commit 7573312f2ad955c0af47b365860b6aed8c705460) --- docs/changelog/118435.yaml | 6 ++++ muted-tests.yml | 9 ++++-- .../src/main/resources/scoring.csv-spec | 30 +++++++++++++++++++ .../xpack/esql/analysis/Verifier.java | 9 ------ .../xpack/esql/analysis/VerifierTests.java | 25 ---------------- 5 files changed, 42 insertions(+), 37 deletions(-) create mode 100644 docs/changelog/118435.yaml diff --git a/docs/changelog/118435.yaml b/docs/changelog/118435.yaml new file mode 100644 index 0000000000000..8bccbeb54698d --- /dev/null +++ b/docs/changelog/118435.yaml @@ -0,0 +1,6 @@ +pr: 118435 +summary: '`_score` should not be a reserved attribute in ES|QL' +area: ES|QL +type: enhancement +issues: + - 118460 diff --git a/muted-tests.yml b/muted-tests.yml index f2486026c45b6..c744258a1e42d 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -382,8 +382,11 @@ tests: - class: "org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT" method: "test {scoring.*}" issue: https://github.com/elastic/elasticsearch/issues/117641 -- class: org.elasticsearch.xpack.esql.ccq.MultiClusterSpecIT - method: test {scoring.QstrWithFieldAndScoringSortedEval} +- class: "org.elasticsearch.xpack.esql.qa.mixed.MultilusterEsqlSpecIT" + method: "test {scoring.*}" + issue: https://github.com/elastic/elasticsearch/issues/118460 +- class: "org.elasticsearch.xpack.esql.ccq.MultiClusterSpecIT" + method: "test {scoring.*}" issue: https://github.com/elastic/elasticsearch/issues/117751 - class: org.elasticsearch.search.ccs.CrossClusterIT method: testCancel @@ -463,4 +466,4 @@ tests: issue: https://github.com/elastic/elasticsearch/issues/118514 - class: org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT method: test {grok.OverwriteNameWhere SYNC} - issue: https://github.com/elastic/elasticsearch/issues/118638 + issue: https://github.com/elastic/elasticsearch/issues/118638 \ No newline at end of file diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/scoring.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/scoring.csv-spec index d4c7b8c59fdbc..cb38204a71ab0 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/scoring.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/scoring.csv-spec @@ -283,3 +283,33 @@ book_no:keyword | c_score:double 7350 | 2.0 7140 | 3.0 ; + +QstrScoreManipulation +required_capability: metadata_score +required_capability: qstr_function + +from books metadata _score +| where qstr("title:rings") +| eval _score = _score + 1 +| keep book_no, title, _score +| limit 2; + +book_no:keyword | title:text | _score:double +4023 | A Tolkien Compass: Including J. R. R. Tolkien's Guide to the Names in The Lord of the Rings | 2.6404519081115723 +2714 | Return of the King Being the Third Part of The Lord of the Rings | 2.9239964485168457 +; + +QstrScoreOverride +required_capability: metadata_score +required_capability: qstr_function + +from books metadata _score +| where qstr("title:rings") +| eval _score = "foobar" +| keep book_no, title, _score +| limit 2; + +book_no:keyword | title:text | _score:keyword +4023 | A Tolkien Compass: Including J. R. R. Tolkien's Guide to the Names in The Lord of the Rings | foobar +2714 | Return of the King Being the Third Part of The Lord of the Rings | foobar +; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java index 890a4866ccdde..f01cc265e330b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java @@ -18,7 +18,6 @@ import org.elasticsearch.xpack.esql.core.expression.Expression; import org.elasticsearch.xpack.esql.core.expression.Expressions; import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; -import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute; import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.expression.TypeResolutions; import org.elasticsearch.xpack.esql.core.expression.function.Function; @@ -208,7 +207,6 @@ else if (p instanceof Lookup lookup) { checkJoin(p, failures); }); checkRemoteEnrich(plan, failures); - checkMetadataScoreNameReserved(plan, failures); if (failures.isEmpty()) { checkLicense(plan, licenseState, failures); @@ -222,13 +220,6 @@ else if (p instanceof Lookup lookup) { return failures; } - private static void checkMetadataScoreNameReserved(LogicalPlan p, Set failures) { - // _score can only be set as metadata attribute - if (p.inputSet().stream().anyMatch(a -> MetadataAttribute.SCORE.equals(a.name()) && (a instanceof MetadataAttribute) == false)) { - failures.add(fail(p, "`" + MetadataAttribute.SCORE + "` is a reserved METADATA attribute")); - } - } - private void checkSort(LogicalPlan p, Set failures) { if (p instanceof OrderBy ob) { ob.order().forEach(o -> { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java index 634c9d6543018..4b916106165fb 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java @@ -12,7 +12,6 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.esql.VerificationException; import org.elasticsearch.xpack.esql.action.EsqlCapabilities; -import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.type.EsField; import org.elasticsearch.xpack.esql.core.type.InvalidMappedField; @@ -22,7 +21,6 @@ import org.elasticsearch.xpack.esql.parser.EsqlParser; import org.elasticsearch.xpack.esql.parser.QueryParam; import org.elasticsearch.xpack.esql.parser.QueryParams; -import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; import java.util.ArrayList; import java.util.LinkedHashMap; @@ -1805,29 +1803,6 @@ public void testToDatePeriodToTimeDurationWithInvalidType() { ); } - public void testNonMetadataScore() { - assumeTrue("'METADATA _score' is disabled", EsqlCapabilities.Cap.METADATA_SCORE.isEnabled()); - assertEquals("1:12: `_score` is a reserved METADATA attribute", error("from foo | eval _score = 10")); - - assertEquals( - "1:48: `_score` is a reserved METADATA attribute", - error("from foo metadata _score | where qstr(\"bar\") | eval _score = _score + 1") - ); - } - - public void testScoreRenaming() { - assumeTrue("'METADATA _score' is disabled", EsqlCapabilities.Cap.METADATA_SCORE.isEnabled()); - assertEquals("1:33: `_score` is a reserved METADATA attribute", error("from foo METADATA _id, _score | rename _id as _score")); - - assertTrue(passes("from foo metadata _score | rename _score as foo").stream().anyMatch(a -> a.name().equals("foo"))); - } - - private List passes(String query) { - LogicalPlan logicalPlan = defaultAnalyzer.analyze(parser.createStatement(query)); - assertTrue(logicalPlan.resolved()); - return logicalPlan.output(); - } - public void testIntervalAsString() { // DateTrunc for (String interval : List.of("1 minu", "1 dy", "1.5 minutes", "0.5 days", "minutes 1", "day 5")) {