From 19c6acedb5c2dc85e3da95c76fb2fe31e4c8dde4 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Tue, 10 Oct 2023 15:45:46 -0700 Subject: [PATCH 1/5] ESQL: Improve verifier error for incorrect agg declaration Add specific check for aliased function declarations in stats Fix #100634 --- .../xpack/esql/analysis/Verifier.java | 15 +++++++++++++++ .../xpack/esql/analysis/VerifierTests.java | 10 +++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) 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 59c6e2782b014..e22b3c4638634 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 @@ -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; @@ -89,6 +91,19 @@ Collection 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, "incomplete aggregate function declaration; add parenthesis to [{}]", 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 index 10f134432a0a2..d4a06ea78e3f9 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 @@ -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(); @@ -292,9 +293,16 @@ public void testPeriodAndDurationInEval() { } } + public void testNestedAggField() { + assertEquals("1:27: Unknown column [avg]", error("from test | stats c = avg(avg)")); + } + + public void testUnfinishedAggFunction() { + assertEquals("1:23: incomplete aggregate function declaration; add parenthesis to [avg]", error("from test | stats c = avg")); + } + private String error(String query) { return error(query, defaultAnalyzer); - } private String error(String query, Object... params) { From 02e6ee77adb55a038155680397cb52b8ab3a3c22 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 11 Oct 2023 03:56:59 +0300 Subject: [PATCH 2/5] Update docs/changelog/100650.yaml --- docs/changelog/100650.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/100650.yaml diff --git a/docs/changelog/100650.yaml b/docs/changelog/100650.yaml new file mode 100644 index 0000000000000..4631571029010 --- /dev/null +++ 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: + - 100634 From 2ae1a7fae3e512e709a59bc18e5128fcd215daf7 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 11 Oct 2023 08:14:28 +0300 Subject: [PATCH 3/5] Update docs/changelog/100650.yaml --- docs/changelog/100650.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/100650.yaml b/docs/changelog/100650.yaml index 4631571029010..96d7bc0571403 100644 --- a/docs/changelog/100650.yaml +++ b/docs/changelog/100650.yaml @@ -3,4 +3,4 @@ summary: "ESQL: Improve verifier error for incorrect agg declaration" area: ES|QL type: bug issues: - - 100634 + - 100641 From 7d2533717ca6fc6536a2bd83798e8a3e44b01bf7 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 11 Oct 2023 16:11:07 -0700 Subject: [PATCH 4/5] Address feedback --- .../java/org/elasticsearch/xpack/esql/analysis/Verifier.java | 2 +- .../org/elasticsearch/xpack/esql/analysis/VerifierTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 e22b3c4638634..8d8163bbcf216 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 @@ -98,7 +98,7 @@ else if (p.resolved()) { var child = as.child(); if (child instanceof UnresolvedAttribute u) { failures.add( - fail(child, "incomplete aggregate function declaration; add parenthesis to [{}]", child.sourceText()) + fail(child, "invalid stats declaration; [{}] is not an aggregate function", child.sourceText()) ); } } 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 d4a06ea78e3f9..57c4c273ec558 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 @@ -298,7 +298,7 @@ public void testNestedAggField() { } public void testUnfinishedAggFunction() { - assertEquals("1:23: incomplete aggregate function declaration; add parenthesis to [avg]", error("from test | stats c = avg")); + assertEquals("1:23: invalid stats declaration; [avg] is not an aggregate function", error("from test | stats c = avg")); } private String error(String query) { From 7abe7828d264906e33d29fd0a2672266ca549c0a Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Wed, 11 Oct 2023 16:41:29 -0700 Subject: [PATCH 5/5] Formatting --- .../java/org/elasticsearch/xpack/esql/analysis/Verifier.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 448cc55544b1b..d8e113862ae71 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 @@ -98,9 +98,7 @@ else if (p.resolved()) { 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()) - ); + failures.add(fail(child, "invalid stats declaration; [{}] is not an aggregate function", child.sourceText())); } } }