소스 검색

ESQL: Re-enable logical dependency check (#105860)

* Fix Enrich refs locally in the dependency check
* Re-enable logical dependency check + test for it
* Return server error on failed dependency check
Alexander Spies 1 년 전
부모
커밋
cc8fb4dba9

+ 5 - 0
docs/changelog/105860.yaml

@@ -0,0 +1,5 @@
+pr: 105860
+summary: "ESQL: Re-enable logical dependency check"
+area: ES|QL
+type: bug
+issues: []

+ 6 - 3
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java

@@ -22,11 +22,10 @@ public final class LogicalVerifier {
     /** Verifies the optimized logical plan. */
     public Failures verify(LogicalPlan plan) {
         Failures failures = new Failures();
+        Failures dependencyFailures = new Failures();
 
         plan.forEachUp(p -> {
-            // dependency check
-            // FIXME: re-enable
-            // DEPENDENCY_CHECK.checkPlan(p, failures);
+            DEPENDENCY_CHECK.checkPlan(p, dependencyFailures);
 
             if (failures.hasFailures() == false) {
                 p.forEachExpression(ex -> {
@@ -37,6 +36,10 @@ public final class LogicalVerifier {
             }
         });
 
+        if (dependencyFailures.hasFailures()) {
+            throw new IllegalStateException(dependencyFailures.toString());
+        }
+
         return failures;
     }
 }

+ 10 - 0
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/OptimizerRules.java

@@ -79,6 +79,16 @@ class OptimizerRules {
     }
 
     static class LogicalPlanDependencyCheck extends DependencyConsistency<LogicalPlan> {
+        @Override
+        protected AttributeSet references(LogicalPlan plan) {
+            if (plan instanceof Enrich enrich) {
+                // The enrichFields are NamedExpressions, so we compute their references as well when just calling enrich.references().
+                // But they are not actually referring to attributes from the input plan - only the match field does.
+                return enrich.matchField().references();
+            }
+            return super.references(plan);
+        }
+
         @Override
         protected AttributeSet generates(LogicalPlan logicalPlan) {
             // source-like operators

+ 1 - 2
x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

@@ -3299,7 +3299,6 @@ public class LogicalPlanOptimizerTests extends ESTestCase {
         assertThat(Expressions.names(local.output()), contains(NO_FIELDS.get(0).name(), "x", "language_code", "language_name"));
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/105436")
     public void testPlanSanityCheck() throws Exception {
         var plan = optimizedPlan("""
             from test
@@ -3325,7 +3324,7 @@ public class LogicalPlanOptimizerTests extends ESTestCase {
             )
         );
 
-        VerificationException e = expectThrows(VerificationException.class, () -> logicalOptimizer.optimize(invalidPlan));
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> logicalOptimizer.optimize(invalidPlan));
         assertThat(e.getMessage(), containsString("Plan [OrderBy[[Order[salary"));
         assertThat(e.getMessage(), containsString(" optimized incorrectly due to missing references [salary"));
     }