Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESQL: Improve verifier error for incorrect agg declaration #100650

Merged
merged 6 commits into from
Oct 11, 2023
Merged

ESQL: Improve verifier error for incorrect agg declaration #100650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
19c6ace
ESQL: Improve verifier error for incorrect agg declaration
costin Oct 10, 2023
02e6ee7
Update docs/changelog/100650.yaml
costin Oct 11, 2023
2ae1a7f
Update docs/changelog/100650.yaml
costin Oct 11, 2023
7d25337
Address feedback
costin Oct 11, 2023
cfb6ae9
Merge branch 'main' into fix/100634
costin Oct 11, 2023
7abe782
Formatting
costin Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/100650.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 100650
summary: "ESQL: Improve verifier error for incorrect agg declaration"
area: ES|QL
type: bug
issues:
- 100641
13 changes: 13 additions & 0 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe consolidate this with the verification happening below in lines 125/140? Now we're verifying aggs in two places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are different types of verifications - the first one is around unknown fields the other around resolved fields but incorrectly declared.

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()) {
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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) {
Expand Down