Skip to content

Commit fcbe4bc

Browse files
julian-elasticjulian-elastic
committed
Address code review feedback part 1
1 parent 4d69b79 commit fcbe4bc

File tree

10 files changed

+115
-72
lines changed

10 files changed

+115
-72
lines changed

‎x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Attribute.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ public static boolean datatypeEquals(List<Attribute> left, List<Attribute> right
143143
return false;
144144
}
145145
for (int i = 0; i < left.size(); i++) {
146-
if (left.get(i).dataType().equals(right.get(i).dataType()) == false) {
146+
if (left.get(i).dataType() != right.get(i).dataType()) {
147147
return false;
148148
}
149149
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ private static Batch<LogicalPlan> localOperators() {
8686

8787
public LogicalPlan localOptimize(LogicalPlan plan) {
8888
LogicalPlan optimized = execute(plan);
89-
Failures failures = verifier.verify(optimized, true, plan);
89+
Failures failures = verifier.verify(optimized, true, plan.output());
9090
if (failures.hasFailures()) {
9191
throw new VerificationException(failures);
9292
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizer.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import org.elasticsearch.xpack.esql.VerificationException;
1111
import org.elasticsearch.xpack.esql.common.Failures;
12+
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1213
import org.elasticsearch.xpack.esql.optimizer.rules.physical.local.EnableSpatialDistancePushdown;
1314
import org.elasticsearch.xpack.esql.optimizer.rules.physical.local.InsertFieldExtraction;
1415
import org.elasticsearch.xpack.esql.optimizer.rules.physical.local.ParallelizeTimeSeriesSource;
@@ -42,15 +43,15 @@ public LocalPhysicalPlanOptimizer(LocalPhysicalOptimizerContext context) {
4243
}
4344

4445
public PhysicalPlan localOptimize(PhysicalPlan plan) {
45-
return verify(execute(plan), plan);
46+
return verify(execute(plan), plan.output());
4647
}
4748

48-
PhysicalPlan verify(PhysicalPlan planAfter, PhysicalPlan planBefore) {
49-
Failures failures = verifier.verify(planAfter, true, planBefore);
49+
PhysicalPlan verify(PhysicalPlan optimizedPlan, List<Attribute> expectedOutputAttributes) {
50+
Failures failures = verifier.verify(optimizedPlan, true, expectedOutputAttributes);
5051
if (failures.hasFailures()) {
5152
throw new VerificationException(failures);
5253
}
53-
return planAfter;
54+
return optimizedPlan;
5455
}
5556

5657
@Override

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ public LogicalPlanOptimizer(LogicalOptimizerContext optimizerContext) {
112112
public LogicalPlan optimize(LogicalPlan verified) {
113113
var optimized = execute(verified);
114114

115-
Failures failures = verifier.verify(optimized, false, verified);
115+
Failures failures = verifier.verify(optimized, false, verified.output());
116116
if (failures.hasFailures()) {
117117
throw new VerificationException(failures);
118118
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java‎

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,33 +10,31 @@
1010
import org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware;
1111
import org.elasticsearch.xpack.esql.common.Failures;
1212
import org.elasticsearch.xpack.esql.optimizer.rules.PlanConsistencyChecker;
13+
import org.elasticsearch.xpack.esql.plan.QueryPlan;
1314
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
14-
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
1515

16-
import static org.elasticsearch.xpack.esql.common.Failure.fail;
17-
import static org.elasticsearch.xpack.esql.core.expression.Attribute.datatypeEquals;
18-
19-
public final class LogicalVerifier {
16+
public final class LogicalVerifier extends PostOptimizationPhasePlanVerifier {
2017

2118
public static final LogicalVerifier INSTANCE = new LogicalVerifier();
2219

2320
private LogicalVerifier() {}
2421

25-
/** Verifies the optimized logical plan. */
26-
public Failures verify(LogicalPlan planAfter, boolean skipRemoteEnrichVerification, LogicalPlan planBefore) {
27-
Failures failures = new Failures();
28-
Failures dependencyFailures = new Failures();
29-
22+
@Override
23+
boolean skipVerification(QueryPlan<?> optimizedPlan, boolean skipRemoteEnrichVerification) {
3024
if (skipRemoteEnrichVerification) {
3125
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531
32-
var enriches = planAfter.collectFirstChildren(Enrich.class::isInstance);
26+
var enriches = optimizedPlan.collectFirstChildren(Enrich.class::isInstance);
3327
if (enriches.isEmpty() == false && ((Enrich) enriches.get(0)).mode() == Enrich.Mode.REMOTE) {
34-
return failures;
28+
return true;
3529
}
3630
}
31+
return false;
32+
}
3733

38-
planAfter.forEachUp(p -> {
39-
PlanConsistencyChecker.checkPlan(p, dependencyFailures);
34+
@Override
35+
void checkPlanConsistency(QueryPlan<?> optimizedPlan, Failures failures, Failures depFailures) {
36+
optimizedPlan.forEachUp(p -> {
37+
PlanConsistencyChecker.checkPlan(p, depFailures);
4038

4139
if (failures.hasFailures() == false) {
4240
if (p instanceof PostOptimizationVerificationAware pova) {
@@ -49,17 +47,5 @@ public Failures verify(LogicalPlan planAfter, boolean skipRemoteEnrichVerificati
4947
});
5048
}
5149
});
52-
53-
if (datatypeEquals(planBefore.output(), planAfter.output()) == false) {
54-
failures.add(
55-
fail(planAfter, "Layout has changed from [{}] to [{}]. ", planBefore.output().toString(), planAfter.output().toString())
56-
);
57-
}
58-
59-
if (dependencyFailures.hasFailures()) {
60-
throw new IllegalStateException(dependencyFailures.toString());
61-
}
62-
63-
return failures;
6450
}
6551
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizer.java‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import org.elasticsearch.xpack.esql.VerificationException;
1111
import org.elasticsearch.xpack.esql.common.Failures;
12+
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1213
import org.elasticsearch.xpack.esql.optimizer.rules.physical.ProjectAwayColumns;
1314
import org.elasticsearch.xpack.esql.plan.physical.FragmentExec;
1415
import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan;
@@ -34,15 +35,15 @@ public PhysicalPlanOptimizer(PhysicalOptimizerContext context) {
3435
}
3536

3637
public PhysicalPlan optimize(PhysicalPlan plan) {
37-
return verify(execute(plan), plan);
38+
return verify(execute(plan), plan.output());
3839
}
3940

40-
PhysicalPlan verify(PhysicalPlan planAfter, PhysicalPlan planBefore) {
41-
Failures failures = verifier.verify(planAfter, false, planBefore);
41+
PhysicalPlan verify(PhysicalPlan optimizedPlan, List<Attribute> expectedOutputAttributes) {
42+
Failures failures = verifier.verify(optimizedPlan, false, expectedOutputAttributes);
4243
if (failures.hasFailures()) {
4344
throw new VerificationException(failures);
4445
}
45-
return planAfter;
46+
return optimizedPlan;
4647
}
4748

4849
@Override

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PhysicalVerifier.java‎

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -12,37 +12,35 @@
1212
import org.elasticsearch.xpack.esql.core.expression.Attribute;
1313
import org.elasticsearch.xpack.esql.core.expression.Expressions;
1414
import org.elasticsearch.xpack.esql.optimizer.rules.PlanConsistencyChecker;
15+
import org.elasticsearch.xpack.esql.plan.QueryPlan;
1516
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
1617
import org.elasticsearch.xpack.esql.plan.physical.EnrichExec;
17-
import org.elasticsearch.xpack.esql.plan.physical.EsQueryExec;
1818
import org.elasticsearch.xpack.esql.plan.physical.FieldExtractExec;
19-
import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan;
2019

21-
import static org.elasticsearch.index.IndexMode.LOOKUP;
2220
import static org.elasticsearch.xpack.esql.common.Failure.fail;
23-
import static org.elasticsearch.xpack.esql.core.expression.Attribute.datatypeEquals;
2421

2522
/** Physical plan verifier. */
26-
public final class PhysicalVerifier {
23+
public final class PhysicalVerifier extends PostOptimizationPhasePlanVerifier {
2724

2825
public static final PhysicalVerifier INSTANCE = new PhysicalVerifier();
2926

3027
private PhysicalVerifier() {}
3128

32-
/** Verifies the physical plan. */
33-
public Failures verify(PhysicalPlan planAfter, boolean skipRemoteEnrichVerification, PhysicalPlan planBefore) {
34-
Failures failures = new Failures();
35-
Failures depFailures = new Failures();
36-
29+
@Override
30+
boolean skipVerification(QueryPlan<?> optimizedPlan, boolean skipRemoteEnrichVerification) {
3731
if (skipRemoteEnrichVerification) {
3832
// AwaitsFix https://github.com/elastic/elasticsearch/issues/118531
39-
var enriches = planAfter.collectFirstChildren(EnrichExec.class::isInstance);
33+
var enriches = optimizedPlan.collectFirstChildren(EnrichExec.class::isInstance);
4034
if (enriches.isEmpty() == false && ((EnrichExec) enriches.get(0)).mode() == Enrich.Mode.REMOTE) {
41-
return failures;
35+
return true;
4236
}
4337
}
38+
return false;
39+
}
4440

45-
planAfter.forEachDown(p -> {
41+
@Override
42+
void checkPlanConsistency(QueryPlan<?> optimizedPlan, Failures failures, Failures depFailures) {
43+
optimizedPlan.forEachDown(p -> {
4644
if (p instanceof FieldExtractExec fieldExtractExec) {
4745
Attribute sourceAttribute = fieldExtractExec.sourceAttribute();
4846
if (sourceAttribute == null) {
@@ -69,19 +67,5 @@ public Failures verify(PhysicalPlan planAfter, boolean skipRemoteEnrichVerificat
6967
});
7068
}
7169
});
72-
73-
if (datatypeEquals(planBefore.output(), planAfter.output()) == false) {
74-
if ((planAfter instanceof EsQueryExec esQueryExec && esQueryExec.indexMode() == LOOKUP) == false) {
75-
failures.add(
76-
fail(planAfter, "Layout has changed from [{}] to [{}]. ", planBefore.output().toString(), planAfter.output().toString())
77-
);
78-
}
79-
}
80-
81-
if (depFailures.hasFailures()) {
82-
throw new IllegalStateException(depFailures.toString());
83-
}
84-
85-
return failures;
8670
}
8771
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/PostOptimizationPhasePlanVerifier.java‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
package org.elasticsearch.xpack.esql.optimizer;
9+
10+
import org.elasticsearch.xpack.esql.common.Failures;
11+
import org.elasticsearch.xpack.esql.core.expression.Attribute;
12+
import org.elasticsearch.xpack.esql.plan.QueryPlan;
13+
import org.elasticsearch.xpack.esql.plan.physical.EsQueryExec;
14+
15+
import java.util.List;
16+
17+
import static org.elasticsearch.index.IndexMode.LOOKUP;
18+
import static org.elasticsearch.xpack.esql.common.Failure.fail;
19+
import static org.elasticsearch.xpack.esql.core.expression.Attribute.datatypeEquals;
20+
21+
/**
22+
* Verifies the plan after optimization.
23+
* This is invoked immediately after a Plan Optimizer completes its work.
24+
* Currently, it is called after LogicalPlanOptimizer, PhysicalPlanOptimizer,
25+
* LocalLogicalPlanOptimizer, and LocalPhysicalPlanOptimizer.
26+
* Note: Logical and Physical optimizers may override methods in this class to perform different checks.
27+
*/
28+
public abstract class PostOptimizationPhasePlanVerifier {
29+
30+
/** Verifies the optimized plan */
31+
public Failures verify(QueryPlan<?> optimizedPlan, boolean skipRemoteEnrichVerification, List<Attribute> expectedOutputAttributes) {
32+
Failures failures = new Failures();
33+
Failures depFailures = new Failures();
34+
if (skipVerification(optimizedPlan, skipRemoteEnrichVerification)) {
35+
return failures;
36+
}
37+
38+
checkPlanConsistency(optimizedPlan, failures, depFailures);
39+
40+
verifyOutputNotChanged(optimizedPlan, expectedOutputAttributes, failures);
41+
42+
if (depFailures.hasFailures()) {
43+
throw new IllegalStateException(depFailures.toString());
44+
}
45+
46+
return failures;
47+
}
48+
49+
abstract boolean skipVerification(QueryPlan<?> optimizedPlan, boolean skipRemoteEnrichVerification);
50+
51+
abstract void checkPlanConsistency(QueryPlan<?> optimizedPlan, Failures failures, Failures depFailures);
52+
53+
private static void verifyOutputNotChanged(QueryPlan<?> optimizedPlan, List<Attribute> expectedOutputAttributes, Failures failures) {
54+
if (datatypeEquals(expectedOutputAttributes, optimizedPlan.output()) == false) {
55+
// LookupJoinExec represents the lookup index with EsSourceExec and this is turned into EsQueryExec by
56+
// ReplaceSourceAttributes. Because InsertFieldExtractions doesn't apply to lookup indices, the
57+
// right hand side will only have the EsQueryExec providing the _doc attribute and nothing else.
58+
if ((optimizedPlan instanceof EsQueryExec esQueryExec && esQueryExec.indexMode() == LOOKUP) == false) {
59+
failures.add(
60+
fail(
61+
optimizedPlan,
62+
"Output has changed from [{}] to [{}]. ",
63+
expectedOutputAttributes.toString(),
64+
optimizedPlan.output().toString()
65+
)
66+
);
67+
}
68+
}
69+
}
70+
71+
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5560,7 +5560,7 @@ public void testPushShadowingGeneratingPlanPastProject() {
55605560
List<Attribute> initialGeneratedExprs = ((GeneratingPlan) initialPlan).generatedAttributes();
55615561
LogicalPlan optimizedPlan = testCase.rule.apply(initialPlan);
55625562

5563-
Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan);
5563+
Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan.output());
55645564
assertFalse(inconsistencies.hasFailures());
55655565

55665566
Project project = as(optimizedPlan, Project.class);
@@ -5611,7 +5611,7 @@ public void testPushShadowingGeneratingPlanPastRenamingProject() {
56115611
List<Attribute> initialGeneratedExprs = ((GeneratingPlan) initialPlan).generatedAttributes();
56125612
LogicalPlan optimizedPlan = testCase.rule.apply(initialPlan);
56135613

5614-
Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan);
5614+
Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan.output());
56155615
assertFalse(inconsistencies.hasFailures());
56165616

56175617
Project project = as(optimizedPlan, Project.class);
@@ -5667,7 +5667,7 @@ public void testPushShadowingGeneratingPlanPastRenamingProjectWithResolution() {
56675667

56685668
// This ensures that our generating plan doesn't use invalid references, resp. that any rename from the Project has
56695669
// been propagated into the generating plan.
5670-
Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan);
5670+
Failures inconsistencies = LogicalVerifier.INSTANCE.verify(optimizedPlan, false, initialPlan.output());
56715671
assertFalse(inconsistencies.hasFailures());
56725672

56735673
Project project = as(optimizedPlan, Project.class);

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/PhysicalPlanOptimizerTests.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2866,7 +2866,7 @@ public void testFieldExtractWithoutSourceAttributes() {
28662866
)
28672867
);
28682868

2869-
var e = expectThrows(VerificationException.class, () -> physicalPlanOptimizer.verify(badPlan, verifiedPlan));
2869+
var e = expectThrows(VerificationException.class, () -> physicalPlanOptimizer.verify(badPlan, verifiedPlan.output()));
28702870
assertThat(
28712871
e.getMessage(),
28722872
containsString(
@@ -2895,7 +2895,7 @@ public void testVerifierOnMissingReferences() {
28952895
)
28962896
);
28972897
final var finalPlan = plan;
2898-
var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan, planBeforeModification));
2898+
var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan, planBeforeModification.output()));
28992899
assertThat(e.getMessage(), containsString(" > 10[INTEGER]]] optimized incorrectly due to missing references [emp_no{f}#"));
29002900
}
29012901

@@ -2913,7 +2913,7 @@ public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception {
29132913

29142914
var planWithInvalidJoinLeftSide = plan.transformUp(LookupJoinExec.class, join -> join.replaceChildren(join.right(), join.right()));
29152915

2916-
var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinLeftSide, plan));
2916+
var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinLeftSide, plan.output()));
29172917
assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from left hand side [languages"));
29182918

29192919
var planWithInvalidJoinRightSide = plan.transformUp(
@@ -2930,7 +2930,7 @@ public void testVerifierOnMissingReferencesWithBinaryPlans() throws Exception {
29302930
)
29312931
);
29322932

2933-
e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinRightSide, plan));
2933+
e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(planWithInvalidJoinRightSide, plan.output()));
29342934
assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references from right hand side [language_code"));
29352935
}
29362936

@@ -2955,7 +2955,7 @@ public void testVerifierOnDuplicateOutputAttributes() {
29552955
);
29562956
});
29572957
final var finalPlan = plan;
2958-
var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan, planBeforeModification));
2958+
var e = expectThrows(IllegalStateException.class, () -> physicalPlanOptimizer.verify(finalPlan, planBeforeModification.output()));
29592959
assertThat(
29602960
e.getMessage(),
29612961
containsString("Plan [LimitExec[1000[INTEGER],null]] optimized incorrectly due to duplicate output attribute emp_no{f}#")

0 commit comments

Comments
 (0)