From bcf0892cecb4364d5594af07ae8a7861465efe19 Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Tue, 1 Jul 2025 15:26:36 -0400 Subject: [PATCH 1/8] Test adding checks --- .../optimizer/LocalLogicalPlanOptimizer.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java index dab2b35025aa6..0e9477513d7e5 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java @@ -7,6 +7,8 @@ package org.elasticsearch.xpack.esql.optimizer; +import org.elasticsearch.xpack.esql.VerificationException; +import org.elasticsearch.xpack.esql.common.Failures; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PropagateEmptyRelation; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStatsFilteredAggWithEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceStringCasingWithInsensitiveRegexMatch; @@ -35,6 +37,8 @@ */ public class LocalLogicalPlanOptimizer extends ParameterizedRuleExecutor { + private final LogicalVerifier verifier = LogicalVerifier.INSTANCE; + private static final List> RULES = arrayAsArrayList( new Batch<>( "Local rewrite", @@ -81,6 +85,16 @@ private static Batch localOperators() { } public LogicalPlan localOptimize(LogicalPlan plan) { - return execute(plan); + Failures failures = verifier.verify(plan); + if (failures.hasFailures()) { + throw new VerificationException("Plan before optimize not valid: " + failures + " for plan: " + plan.nodeString()); + } + LogicalPlan optimized = execute(plan); + failures = verifier.verify(optimized); + if (failures.hasFailures()) { + throw new VerificationException("Plan after optimize not valid: " + failures + " for plan: " + plan.nodeString()); + } + return optimized; } + } From 4076106b0b74c19bb251ee7667b02e989b73b7d9 Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Wed, 2 Jul 2025 10:31:59 -0400 Subject: [PATCH 2/8] Test adding checks --- .../xpack/esql/optimizer/LocalLogicalPlanOptimizer.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java index 0e9477513d7e5..dee95479c04d3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java @@ -85,12 +85,8 @@ private static Batch localOperators() { } public LogicalPlan localOptimize(LogicalPlan plan) { - Failures failures = verifier.verify(plan); - if (failures.hasFailures()) { - throw new VerificationException("Plan before optimize not valid: " + failures + " for plan: " + plan.nodeString()); - } LogicalPlan optimized = execute(plan); - failures = verifier.verify(optimized); + Failures failures = verifier.verify(optimized); if (failures.hasFailures()) { throw new VerificationException("Plan after optimize not valid: " + failures + " for plan: " + plan.nodeString()); } From 4ae12021f839cd34d84d2490d413feddcf9a9a41 Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Thu, 3 Jul 2025 08:56:45 -0400 Subject: [PATCH 3/8] Disable for remote enrich --- .../xpack/esql/optimizer/LogicalVerifier.java | 7 +++++++ 1 file changed, 7 insertions(+) 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 c474c48d6d96b..91a736c7eafd1 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 @@ -10,6 +10,7 @@ import org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware; import org.elasticsearch.xpack.esql.common.Failures; import org.elasticsearch.xpack.esql.optimizer.rules.PlanConsistencyChecker; +import org.elasticsearch.xpack.esql.plan.logical.Enrich; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; public final class LogicalVerifier { @@ -23,6 +24,12 @@ public Failures verify(LogicalPlan plan) { Failures failures = new Failures(); Failures dependencyFailures = new Failures(); + // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 + var enriches = plan.collectFirstChildren(Enrich.class::isInstance); + if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { + return failures; + } + plan.forEachUp(p -> { PlanConsistencyChecker.checkPlan(p, dependencyFailures); From 68a42621bf7c8a58b0ba704ed7fe4f4da2fd9fa1 Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Thu, 3 Jul 2025 10:34:39 -0400 Subject: [PATCH 4/8] Update docs/changelog/130409.yaml --- docs/changelog/130409.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/130409.yaml diff --git a/docs/changelog/130409.yaml b/docs/changelog/130409.yaml new file mode 100644 index 0000000000000..55dfd1c42e181 --- /dev/null +++ b/docs/changelog/130409.yaml @@ -0,0 +1,5 @@ +pr: 130409 +summary: Add Dependency Checker for `LogicalLocalPlanOptimizer` +area: ES|QL +type: enhancement +issues: [] From bd6b731ac3ac51ec59e7bc8f791ca42edd07a649 Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Mon, 7 Jul 2025 10:21:21 -0400 Subject: [PATCH 5/8] Address some of the code review comments --- .../optimizer/LocalLogicalPlanOptimizer.java | 2 +- .../LocalLogicalPlanOptimizerTests.java | 32 +++++++++++++++++++ .../LocalPhysicalPlanOptimizerTests.java | 27 ++++++++++++++++ .../esql/optimizer/TestPlannerOptimizer.java | 13 ++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java index dee95479c04d3..fc87cf2faf84d 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java @@ -88,7 +88,7 @@ public LogicalPlan localOptimize(LogicalPlan plan) { LogicalPlan optimized = execute(plan); Failures failures = verifier.verify(optimized); if (failures.hasFailures()) { - throw new VerificationException("Plan after optimize not valid: " + failures + " for plan: " + plan.nodeString()); + throw new VerificationException(failures); } return optimized; } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java index f2b70c99253b8..28107e6e3dadf 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java @@ -23,13 +23,16 @@ import org.elasticsearch.xpack.esql.core.expression.FieldAttribute; import org.elasticsearch.xpack.esql.core.expression.FoldContext; import org.elasticsearch.xpack.esql.core.expression.Literal; +import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.core.type.DataType; import org.elasticsearch.xpack.esql.core.type.EsField; import org.elasticsearch.xpack.esql.core.type.InvalidMappedField; +import org.elasticsearch.xpack.esql.expression.Order; import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry; +import org.elasticsearch.xpack.esql.expression.function.aggregate.Min; import org.elasticsearch.xpack.esql.expression.function.scalar.conditional.Case; import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce; import org.elasticsearch.xpack.esql.expression.function.scalar.string.StartsWith; @@ -49,6 +52,7 @@ import org.elasticsearch.xpack.esql.plan.logical.Limit; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.esql.plan.logical.MvExpand; +import org.elasticsearch.xpack.esql.plan.logical.OrderBy; import org.elasticsearch.xpack.esql.plan.logical.Project; import org.elasticsearch.xpack.esql.plan.logical.Row; import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; @@ -64,6 +68,7 @@ import java.util.Map; import java.util.Set; +import static java.util.Arrays.asList; import static java.util.Collections.emptyMap; import static org.elasticsearch.xpack.esql.EsqlTestUtils.L; import static org.elasticsearch.xpack.esql.EsqlTestUtils.ONE; @@ -84,6 +89,7 @@ import static org.elasticsearch.xpack.esql.EsqlTestUtils.withDefaultLimitWarning; import static org.elasticsearch.xpack.esql.core.tree.Source.EMPTY; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; @@ -774,6 +780,32 @@ public void testGroupingByMissingFields() { as(eval.child(), EsRelation.class); } + public void testPlanSanityCheck() throws Exception { + var plan = localPlan(""" + from test + | stats a = min(salary) by emp_no + """); + + var limit = as(plan, Limit.class); + var aggregate = as(limit.child(), Aggregate.class); + var min = as(Alias.unwrap(aggregate.aggregates().get(0)), Min.class); + var salary = as(min.field(), NamedExpression.class); + assertThat(salary.name(), is("salary")); + // emulate a rule that adds an invalid field + var invalidPlan = new OrderBy( + limit.source(), + limit, + asList(new Order(limit.source(), salary, Order.OrderDirection.ASC, Order.NullsPosition.FIRST)) + ); + + var localContext = new LocalLogicalOptimizerContext(EsqlTestUtils.TEST_CFG, FoldContext.small(), TEST_SEARCH_STATS); + LocalLogicalPlanOptimizer localLogicalPlanOptimizer = new LocalLogicalPlanOptimizer(localContext); + + IllegalStateException e = expectThrows(IllegalStateException.class, () -> localLogicalPlanOptimizer.localOptimize(invalidPlan)); + assertThat(e.getMessage(), containsString("Plan [OrderBy[[Order[salary")); + assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references [salary")); + } + private IsNotNull isNotNull(Expression field) { return new IsNotNull(EMPTY, field); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java index 66b797afa426c..169dbb0660f7a 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java @@ -52,9 +52,11 @@ import org.elasticsearch.xpack.esql.core.type.MultiTypeEsField; import org.elasticsearch.xpack.esql.core.util.Holder; import org.elasticsearch.xpack.esql.enrich.ResolvedEnrichPolicy; +import org.elasticsearch.xpack.esql.expression.Order; import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry; import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute; import org.elasticsearch.xpack.esql.expression.function.aggregate.Count; +import org.elasticsearch.xpack.esql.expression.function.aggregate.Min; import org.elasticsearch.xpack.esql.expression.function.fulltext.FullTextFunction; import org.elasticsearch.xpack.esql.expression.function.fulltext.Kql; import org.elasticsearch.xpack.esql.expression.function.fulltext.Match; @@ -131,8 +133,12 @@ import static org.elasticsearch.xpack.esql.analysis.AnalyzerTestUtils.indexWithDateDateNanosUnionType; import static org.elasticsearch.xpack.esql.core.querydsl.query.Query.unscore; import static org.elasticsearch.xpack.esql.core.type.DataType.DATE_NANOS; +import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER; +import static org.elasticsearch.xpack.esql.core.util.TestUtils.getFieldAttribute; +import static org.elasticsearch.xpack.esql.plan.physical.AbstractPhysicalPlanSerializationTests.randomEstimatedRowSize; import static org.elasticsearch.xpack.esql.plan.physical.EsStatsQueryExec.StatsType; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; @@ -2056,6 +2062,27 @@ public void testToDateNanosPushDown() { assertThat(expected.toString(), is(esQuery.query().toString())); } + public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception { + + PhysicalPlan plan = plannerOptimizer.plan(""" + from test + | stats a = min(salary) by emp_no + """); + + var limit = as(plan, LimitExec.class); + var aggregate = as(limit.child(), AggregateExec.class); + var min = as(Alias.unwrap(aggregate.aggregates().get(0)), Min.class); + var salary = as(min.field(), NamedExpression.class); + assertThat(salary.name(), is("salary")); + // emulate a rule that adds a missing attribute + FieldAttribute missingAttr = getFieldAttribute("missing attr"); + List orders = List.of(new Order(plan.source(), missingAttr, Order.OrderDirection.ASC, Order.NullsPosition.FIRST)); + TopNExec topNExec = new TopNExec(plan.source(), plan, orders, new Literal(Source.EMPTY, limit, INTEGER), randomEstimatedRowSize()); + + Exception e = expectThrows(IllegalStateException.class, () -> plannerOptimizer.localPhysicalVerify(topNExec)); + assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references [missing attr")); + } + private boolean isMultiTypeEsField(Expression e) { return e instanceof FieldAttribute fa && fa.field() instanceof MultiTypeEsField; } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java index e6a7d110f8c09..122edefa49656 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java @@ -23,6 +23,8 @@ public class TestPlannerOptimizer { private final Analyzer analyzer; private final LogicalPlanOptimizer logicalOptimizer; private final PhysicalPlanOptimizer physicalPlanOptimizer; + private final LocalPhysicalPlanOptimizer localPhysicalPlanOptimizer; + private final Mapper mapper; private final Configuration config; @@ -38,6 +40,9 @@ public TestPlannerOptimizer(Configuration config, Analyzer analyzer, LogicalPlan parser = new EsqlParser(); physicalPlanOptimizer = new PhysicalPlanOptimizer(new PhysicalOptimizerContext(config)); mapper = new Mapper(); + localPhysicalPlanOptimizer = new LocalPhysicalPlanOptimizer( + new LocalPhysicalOptimizerContext(config, FoldContext.small(), SearchStats.EMPTY) + ); } @@ -84,4 +89,12 @@ private PhysicalPlan physicalPlan(String query, Analyzer analyzer) { var physical = mapper.map(logical); return physical; } + + public PhysicalPlan localPhysicalOptimize(PhysicalPlan plan) { + return localPhysicalPlanOptimizer.localOptimize(plan); + } + + public PhysicalPlan localPhysicalVerify(PhysicalPlan plan) { + return localPhysicalPlanOptimizer.verify(plan); + } } From 3fdc7cd50b6da5c2033891c388a266f264a61b16 Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Mon, 7 Jul 2025 12:06:49 -0400 Subject: [PATCH 6/8] Limit skipRemoteEnrichVerification to Local planning --- .../esql/optimizer/LocalLogicalPlanOptimizer.java | 2 +- .../esql/optimizer/LocalPhysicalPlanOptimizer.java | 2 +- .../xpack/esql/optimizer/LogicalPlanOptimizer.java | 2 +- .../xpack/esql/optimizer/LogicalVerifier.java | 12 +++++++----- .../xpack/esql/optimizer/PhysicalPlanOptimizer.java | 2 +- .../xpack/esql/optimizer/PhysicalVerifier.java | 12 +++++++----- .../esql/optimizer/LogicalPlanOptimizerTests.java | 6 +++--- 7 files changed, 21 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java index fc87cf2faf84d..b9d85d191f1d2 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java @@ -86,7 +86,7 @@ private static Batch localOperators() { public LogicalPlan localOptimize(LogicalPlan plan) { LogicalPlan optimized = execute(plan); - Failures failures = verifier.verify(optimized); + Failures failures = verifier.verify(optimized, true); if (failures.hasFailures()) { throw new VerificationException(failures); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java index b3fb1aa7098e9..836eab9bb9590 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java @@ -46,7 +46,7 @@ public PhysicalPlan localOptimize(PhysicalPlan plan) { } PhysicalPlan verify(PhysicalPlan plan) { - Failures failures = verifier.verify(plan); + Failures failures = verifier.verify(plan, true); if (failures.hasFailures()) { throw new VerificationException(failures); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index 14a858f85fd2a..eed6a6b57b68f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -112,7 +112,7 @@ public LogicalPlanOptimizer(LogicalOptimizerContext optimizerContext) { public LogicalPlan optimize(LogicalPlan verified) { var optimized = execute(verified); - Failures failures = verifier.verify(optimized); + Failures failures = verifier.verify(optimized, false); if (failures.hasFailures()) { throw new VerificationException(failures); } 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 91a736c7eafd1..13627ed759e8a 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 @@ -20,14 +20,16 @@ public final class LogicalVerifier { private LogicalVerifier() {} /** Verifies the optimized logical plan. */ - public Failures verify(LogicalPlan plan) { + public Failures verify(LogicalPlan plan, boolean skipRemoteEnrichVerification) { Failures failures = new Failures(); Failures dependencyFailures = new Failures(); - // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 - var enriches = plan.collectFirstChildren(Enrich.class::isInstance); - if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { - return failures; + if(skipRemoteEnrichVerification) { + // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 + var enriches = plan.collectFirstChildren(Enrich.class::isInstance); + if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { + return failures; + } } plan.forEachUp(p -> { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java index 57e2c8cba4c32..ab6bea5ffddac 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java @@ -38,7 +38,7 @@ public PhysicalPlan optimize(PhysicalPlan plan) { } PhysicalPlan verify(PhysicalPlan plan) { - Failures failures = verifier.verify(plan); + Failures failures = verifier.verify(plan, false); if (failures.hasFailures()) { throw new VerificationException(failures); } 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 629361a40530c..1cbc8aee59961 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 @@ -27,14 +27,16 @@ public final class PhysicalVerifier { private PhysicalVerifier() {} /** Verifies the physical plan. */ - public Failures verify(PhysicalPlan plan) { + public Failures verify(PhysicalPlan plan, boolean skipRemoteEnrichVerification) { Failures failures = new Failures(); Failures depFailures = new Failures(); - // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 - var enriches = plan.collectFirstChildren(EnrichExec.class::isInstance); - if (enriches.isEmpty() == false && ((EnrichExec) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { - return failures; + if (skipRemoteEnrichVerification){ + // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 + var enriches = plan.collectFirstChildren(EnrichExec.class::isInstance); + if (enriches.isEmpty() == false && ((EnrichExec) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { + return failures; + } } plan.forEachDown(p -> { 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 7795bf5c2d9ff..f6a4cf3ee945a 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 @@ -5554,7 +5554,7 @@ public void testPushShadowingGeneratingPlanPastProject() { List initialGeneratedExprs = ((GeneratingPlan) initialPlan).generatedAttributes(); LogicalPlan optimizedPlan = testCase.rule.apply(initialPlan); - Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan); + Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false); assertFalse(inconsistencies.hasFailures()); Project project = as(optimizedPlan, Project.class); @@ -5605,7 +5605,7 @@ public void testPushShadowingGeneratingPlanPastRenamingProject() { List initialGeneratedExprs = ((GeneratingPlan) initialPlan).generatedAttributes(); LogicalPlan optimizedPlan = testCase.rule.apply(initialPlan); - Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan); + Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false); assertFalse(inconsistencies.hasFailures()); Project project = as(optimizedPlan, Project.class); @@ -5661,7 +5661,7 @@ public void testPushShadowingGeneratingPlanPastRenamingProjectWithResolution() { // This ensures that our generating plan doesn't use invalid references, resp. that any rename from the Project has // been propagated into the generating plan. - Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan); + Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false); assertFalse(inconsistencies.hasFailures()); Project project = as(optimizedPlan, Project.class); From 88aecdbbae0c7e9b91bd9e94f6dd9b5c5f59bc7a Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Mon, 7 Jul 2025 12:50:10 -0400 Subject: [PATCH 7/8] Make sure verify is being called when we call LocalPhysicalPlanOptimizer.localOptimize() --- .../xpack/esql/optimizer/LogicalVerifier.java | 2 +- .../xpack/esql/optimizer/PhysicalVerifier.java | 2 +- .../LocalPhysicalPlanOptimizerTests.java | 15 +++++++++++++-- .../esql/optimizer/TestPlannerOptimizer.java | 13 ------------- 4 files changed, 15 insertions(+), 17 deletions(-) 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 13627ed759e8a..6751ae4cd2d80 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 @@ -24,7 +24,7 @@ public Failures verify(LogicalPlan plan, boolean skipRemoteEnrichVerification) { Failures failures = new Failures(); Failures dependencyFailures = new Failures(); - if(skipRemoteEnrichVerification) { + if (skipRemoteEnrichVerification) { // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 var enriches = plan.collectFirstChildren(Enrich.class::isInstance); if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { 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 1cbc8aee59961..607aa11575bcb 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 @@ -31,7 +31,7 @@ public Failures verify(PhysicalPlan plan, boolean skipRemoteEnrichVerification) Failures failures = new Failures(); Failures depFailures = new Failures(); - if (skipRemoteEnrichVerification){ + if (skipRemoteEnrichVerification) { // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 var enriches = plan.collectFirstChildren(EnrichExec.class::isInstance); if (enriches.isEmpty() == false && ((EnrichExec) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java index 169dbb0660f7a..d1319656f6801 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java @@ -43,6 +43,7 @@ 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.FoldContext; import org.elasticsearch.xpack.esql.core.expression.Literal; import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.expression.ReferenceAttribute; @@ -2062,7 +2063,7 @@ public void testToDateNanosPushDown() { assertThat(expected.toString(), is(esQuery.query().toString())); } - public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception { + public void testVerifierOnMissingReferences() throws Exception { PhysicalPlan plan = plannerOptimizer.plan(""" from test @@ -2079,7 +2080,17 @@ public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception { List orders = List.of(new Order(plan.source(), missingAttr, Order.OrderDirection.ASC, Order.NullsPosition.FIRST)); TopNExec topNExec = new TopNExec(plan.source(), plan, orders, new Literal(Source.EMPTY, limit, INTEGER), randomEstimatedRowSize()); - Exception e = expectThrows(IllegalStateException.class, () -> plannerOptimizer.localPhysicalVerify(topNExec)); + // We want to verify that the localOptimize detects the missing attribute. + // However, it also throws an error in one of the rules before we get to the verifier. + // So we use an implementation of LocalPhysicalPlanOptimizer that does not have any rules. + LocalPhysicalOptimizerContext context = new LocalPhysicalOptimizerContext(config, FoldContext.small(), SearchStats.EMPTY); + LocalPhysicalPlanOptimizer optimizerWithNoopExecute = new LocalPhysicalPlanOptimizer(context) { + @Override + protected List> batches() { + return List.of(); + } + }; + Exception e = expectThrows(IllegalStateException.class, () -> optimizerWithNoopExecute.localOptimize(topNExec)); assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references [missing attr")); } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java index 122edefa49656..e6a7d110f8c09 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/TestPlannerOptimizer.java @@ -23,8 +23,6 @@ public class TestPlannerOptimizer { private final Analyzer analyzer; private final LogicalPlanOptimizer logicalOptimizer; private final PhysicalPlanOptimizer physicalPlanOptimizer; - private final LocalPhysicalPlanOptimizer localPhysicalPlanOptimizer; - private final Mapper mapper; private final Configuration config; @@ -40,9 +38,6 @@ public TestPlannerOptimizer(Configuration config, Analyzer analyzer, LogicalPlan parser = new EsqlParser(); physicalPlanOptimizer = new PhysicalPlanOptimizer(new PhysicalOptimizerContext(config)); mapper = new Mapper(); - localPhysicalPlanOptimizer = new LocalPhysicalPlanOptimizer( - new LocalPhysicalOptimizerContext(config, FoldContext.small(), SearchStats.EMPTY) - ); } @@ -89,12 +84,4 @@ private PhysicalPlan physicalPlan(String query, Analyzer analyzer) { var physical = mapper.map(logical); return physical; } - - public PhysicalPlan localPhysicalOptimize(PhysicalPlan plan) { - return localPhysicalPlanOptimizer.localOptimize(plan); - } - - public PhysicalPlan localPhysicalVerify(PhysicalPlan plan) { - return localPhysicalPlanOptimizer.verify(plan); - } } From 5bd41dd7f508c63b6e7bf92d9a4c34a187b3629a Mon Sep 17 00:00:00 2001 From: Julian Kiryakov Date: Mon, 7 Jul 2025 13:40:06 -0400 Subject: [PATCH 8/8] Add checks that optimizers do not modify the layout --- .../xpack/esql/core/expression/Attribute.java | 12 ++++++++++++ .../esql/optimizer/LocalLogicalPlanOptimizer.java | 2 +- .../optimizer/LocalPhysicalPlanOptimizer.java | 8 ++++---- .../esql/optimizer/LogicalPlanOptimizer.java | 2 +- .../xpack/esql/optimizer/LogicalVerifier.java | 15 ++++++++++++--- .../esql/optimizer/PhysicalPlanOptimizer.java | 8 ++++---- .../xpack/esql/optimizer/PhysicalVerifier.java | 12 +++++++++--- .../esql/optimizer/LogicalPlanOptimizerTests.java | 6 +++--- .../optimizer/PhysicalPlanOptimizerTests.java | 14 +++++++------- 9 files changed, 53 insertions(+), 26 deletions(-) diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java index 1af98f4b21dc5..aee05d02bffa7 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java @@ -134,4 +134,16 @@ public String nodeString() { } protected abstract String label(); + + public static boolean semanticEquals(List left, List right) { + if (left.size() != right.size()) { + return false; + } + for (int i = 0; i < left.size(); i++) { + if (left.get(i).semanticEquals(right.get(i)) == false) { + return false; + } + } + return true; + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java index b9d85d191f1d2..c6122fb3746ed 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java @@ -86,7 +86,7 @@ private static Batch localOperators() { public LogicalPlan localOptimize(LogicalPlan plan) { LogicalPlan optimized = execute(plan); - Failures failures = verifier.verify(optimized, true); + Failures failures = verifier.verify(optimized, true, plan); if (failures.hasFailures()) { throw new VerificationException(failures); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java index 836eab9bb9590..47f42450dbaea 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java @@ -42,15 +42,15 @@ public LocalPhysicalPlanOptimizer(LocalPhysicalOptimizerContext context) { } public PhysicalPlan localOptimize(PhysicalPlan plan) { - return verify(execute(plan)); + return verify(execute(plan), plan); } - PhysicalPlan verify(PhysicalPlan plan) { - Failures failures = verifier.verify(plan, true); + PhysicalPlan verify(PhysicalPlan planAfter, PhysicalPlan planBefore) { + Failures failures = verifier.verify(planAfter, true, planBefore); if (failures.hasFailures()) { throw new VerificationException(failures); } - return plan; + return planAfter; } @Override diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index eed6a6b57b68f..345152689753a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -112,7 +112,7 @@ public LogicalPlanOptimizer(LogicalOptimizerContext optimizerContext) { public LogicalPlan optimize(LogicalPlan verified) { var optimized = execute(verified); - Failures failures = verifier.verify(optimized, false); + Failures failures = verifier.verify(optimized, false, verified); if (failures.hasFailures()) { throw new VerificationException(failures); } 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 6751ae4cd2d80..4520f9d591fc3 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 @@ -13,6 +13,9 @@ import org.elasticsearch.xpack.esql.plan.logical.Enrich; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import static org.elasticsearch.xpack.esql.common.Failure.fail; +import static org.elasticsearch.xpack.esql.core.expression.Attribute.semanticEquals; + public final class LogicalVerifier { public static final LogicalVerifier INSTANCE = new LogicalVerifier(); @@ -20,19 +23,19 @@ public final class LogicalVerifier { private LogicalVerifier() {} /** Verifies the optimized logical plan. */ - public Failures verify(LogicalPlan plan, boolean skipRemoteEnrichVerification) { + public Failures verify(LogicalPlan planAfter, boolean skipRemoteEnrichVerification, LogicalPlan planBefore) { Failures failures = new Failures(); Failures dependencyFailures = new Failures(); if (skipRemoteEnrichVerification) { // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 - var enriches = plan.collectFirstChildren(Enrich.class::isInstance); + var enriches = planAfter.collectFirstChildren(Enrich.class::isInstance); if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { return failures; } } - plan.forEachUp(p -> { + planAfter.forEachUp(p -> { PlanConsistencyChecker.checkPlan(p, dependencyFailures); if (failures.hasFailures() == false) { @@ -47,6 +50,12 @@ public Failures verify(LogicalPlan plan, boolean skipRemoteEnrichVerification) { } }); + if (semanticEquals(planBefore.output(), planAfter.output()) == false) { + failures.add( + fail(planAfter, "Layout has changed from [{}] to [{}]. ", planBefore.output().toString(), planAfter.output().toString()) + ); + } + if (dependencyFailures.hasFailures()) { throw new IllegalStateException(dependencyFailures.toString()); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java index ab6bea5ffddac..55b8a43f893c5 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java @@ -34,15 +34,15 @@ public PhysicalPlanOptimizer(PhysicalOptimizerContext context) { } public PhysicalPlan optimize(PhysicalPlan plan) { - return verify(execute(plan)); + return verify(execute(plan), plan); } - PhysicalPlan verify(PhysicalPlan plan) { - Failures failures = verifier.verify(plan, false); + PhysicalPlan verify(PhysicalPlan planAfter, PhysicalPlan planBefore) { + Failures failures = verifier.verify(planAfter, false, planBefore); if (failures.hasFailures()) { throw new VerificationException(failures); } - return plan; + return planAfter; } @Override 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 607aa11575bcb..9cf538b5248ef 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 @@ -27,19 +27,19 @@ public final class PhysicalVerifier { private PhysicalVerifier() {} /** Verifies the physical plan. */ - public Failures verify(PhysicalPlan plan, boolean skipRemoteEnrichVerification) { + public Failures verify(PhysicalPlan planAfter, boolean skipRemoteEnrichVerification, PhysicalPlan planBefore) { Failures failures = new Failures(); Failures depFailures = new Failures(); if (skipRemoteEnrichVerification) { // AwaitsFix https://github.com/elastic/elasticsearch/issues/118531 - var enriches = plan.collectFirstChildren(EnrichExec.class::isInstance); + var enriches = planAfter.collectFirstChildren(EnrichExec.class::isInstance); if (enriches.isEmpty() == false && ((EnrichExec) enriches.get(0)).mode() == Enrich.Mode.REMOTE) { return failures; } } - plan.forEachDown(p -> { + planAfter.forEachDown(p -> { if (p instanceof FieldExtractExec fieldExtractExec) { Attribute sourceAttribute = fieldExtractExec.sourceAttribute(); if (sourceAttribute == null) { @@ -67,6 +67,12 @@ public Failures verify(PhysicalPlan plan, boolean skipRemoteEnrichVerification) } }); + if (planBefore.output().equals(planAfter.output()) == false) { + failures.add( + fail(planAfter, "Layout has changed from [{}] to [{}]. ", planBefore.output().toString(), planAfter.output().toString()) + ); + } + if (depFailures.hasFailures()) { throw new IllegalStateException(depFailures.toString()); } 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 f6a4cf3ee945a..55f1acbf9bb94 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 @@ -5554,7 +5554,7 @@ public void testPushShadowingGeneratingPlanPastProject() { List initialGeneratedExprs = ((GeneratingPlan) initialPlan).generatedAttributes(); LogicalPlan optimizedPlan = testCase.rule.apply(initialPlan); - Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false); + Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan); assertFalse(inconsistencies.hasFailures()); Project project = as(optimizedPlan, Project.class); @@ -5605,7 +5605,7 @@ public void testPushShadowingGeneratingPlanPastRenamingProject() { List initialGeneratedExprs = ((GeneratingPlan) initialPlan).generatedAttributes(); LogicalPlan optimizedPlan = testCase.rule.apply(initialPlan); - Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false); + Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan); assertFalse(inconsistencies.hasFailures()); Project project = as(optimizedPlan, Project.class); @@ -5661,7 +5661,7 @@ public void testPushShadowingGeneratingPlanPastRenamingProjectWithResolution() { // This ensures that our generating plan doesn't use invalid references, resp. that any rename from the Project has // been propagated into the generating plan. - Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false); + Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan); assertFalse(inconsistencies.hasFailures()); Project project = as(optimizedPlan, Project.class); 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 99eded20b1687..f5574db5f3ea5 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 @@ -2865,7 +2865,7 @@ public void testFieldExtractWithoutSourceAttributes() { ) ); - var e = expectThrows(VerificationException.class, () -> physicalPlanOptimizer.verify(badPlan)); + var e = expectThrows(VerificationException.class, () -> physicalPlanOptimizer.verify(badPlan, verifiedPlan)); assertThat( e.getMessage(), containsString( @@ -2880,7 +2880,7 @@ public void testVerifierOnMissingReferences() { | stats s = sum(salary) by emp_no | where emp_no > 10 """); - + final var planBeforeModification = plan; plan = plan.transformUp( AggregateExec.class, a -> new AggregateExec( @@ -2894,7 +2894,7 @@ public void testVerifierOnMissingReferences() { ) ); final var finalPlan = plan; - var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan)); + var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan, planBeforeModification)); assertThat(e.getMessage(), containsString(" > 10[INTEGER]]] optimized incorrectly due to missing references [emp_no{f}#")); } @@ -2912,7 +2912,7 @@ public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception { var planWithInvalidJoinLeftSide = plan.transformUp(LookupJoinExec.class, join -> join.replaceChildren(join.right(), join.right())); - var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinLeftSide)); + var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinLeftSide, plan)); assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from left hand side [languages")); var planWithInvalidJoinRightSide = plan.transformUp( @@ -2929,7 +2929,7 @@ public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception { ) ); - e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinRightSide)); + e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinRightSide, plan)); assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from right hand side [language_code")); } @@ -2939,7 +2939,7 @@ public void testVerifierOnDuplicateOutputAttributes() { | stats s = sum(salary) by emp_no | where emp_no > 10 """); - + final var planBeforeModification = plan; plan = plan.transformUp(AggregateExec.class, a -> { List intermediates = new ArrayList<>(a.intermediateAttributes()); intermediates.add(intermediates.get(0)); @@ -2954,7 +2954,7 @@ public void testVerifierOnDuplicateOutputAttributes() { ); }); final var finalPlan = plan; - var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan)); + var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan, planBeforeModification)); assertThat( e.getMessage(), containsString("Plan [LimitExec[1000[INTEGER],null]] optimized incorrectly due to duplicate output attribute emp_no{f}#")