ESQL: Improve verifier error for incorrect agg declaration (elastic#1…

…00650) Add specific check for aliased function declarations in stats Fix elastic#100634
costin · Oct 11, 2023 · e8d93d4 · e8d93d4
1 parent a6a1ef5
commit e8d93d4
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 1 deletion.
diff --git a/docs/changelog/100650.yaml b/docs/changelog/100650.yaml
@@ -0,0 +1,6 @@
+pr: 100650
+summary: "ESQL: Improve verifier error for incorrect agg declaration"
+area: ES|QL
+type: bug
+issues:
+ - 100641
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
@@ -26,8 +26,10 @@
 import org.elasticsearch.xpack.ql.expression.FieldAttribute;
 import org.elasticsearch.xpack.ql.expression.Literal;
 import org.elasticsearch.xpack.ql.expression.MetadataAttribute;
+import org.elasticsearch.xpack.ql.expression.NamedExpression;
 import org.elasticsearch.xpack.ql.expression.ReferenceAttribute;
 import org.elasticsearch.xpack.ql.expression.TypeResolutions;
+import org.elasticsearch.xpack.ql.expression.UnresolvedAttribute;
 import org.elasticsearch.xpack.ql.expression.function.aggregate.AggregateFunction;
 import org.elasticsearch.xpack.ql.expression.predicate.BinaryOperator;
 import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
@@ -90,6 +92,17 @@ Collection<Failure> verify(LogicalPlan plan) {
             else if (p.resolved()) {
                 return;
             }
+            // handle aggregate first to disambiguate between missing fields or incorrect function declaration
+            if (p instanceof Aggregate aggregate) {
+                for (NamedExpression agg : aggregate.aggregates()) {
+                    if (agg instanceof Alias as) {
+                        var child = as.child();
+                        if (child instanceof UnresolvedAttribute u) {
+                            failures.add(fail(child, "invalid stats declaration; [{}] is not an aggregate function", child.sourceText()));
+                        }
+                    }
+                }
+            }
             p.forEachExpression(e -> {
                 // everything is fine, skip expression
                 if (e.resolved()) {

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
@@ -19,6 +19,7 @@
 import static org.elasticsearch.xpack.ql.type.DataTypes.UNSIGNED_LONG;
 import static org.hamcrest.Matchers.containsString;
 
+//@TestLogging(value = "org.elasticsearch.xpack.esql:TRACE,org.elasticsearch.compute:TRACE", reason = "debug")
 public class VerifierTests extends ESTestCase {
 
     private static final EsqlParser parser = new EsqlParser();
@@ -300,9 +301,16 @@ public void testFilterDateConstant() {
         assertEquals("1:19: Condition expression needs to be boolean, found [DATE_PERIOD]", error("from test | where 1 year"));
     }
 
+    public void testNestedAggField() {
+        assertEquals("1:27: Unknown column [avg]", error("from test | stats c = avg(avg)"));
+    }
+
+    public void testUnfinishedAggFunction() {
+        assertEquals("1:23: invalid stats declaration; [avg] is not an aggregate function", error("from test | stats c = avg"));
+    }
+
     private String error(String query) {
         return error(query, defaultAnalyzer);
-
     }
 
     private String error(String query, Object... params) {