From 6d46fd91f54751d447443a42ebd2ea44a6a41d62 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 9 Feb 2021 15:51:47 +0100 Subject: [PATCH 1/2] Enhance error msg on filtering check against aggs Distinguish between the case where the filtering is a WHERE with aggs appart from a HAVING with missing aggs. --- .../xpack/sql/analysis/analyzer/Verifier.java | 18 ++++++++++++++---- .../analyzer/VerifierErrorMessagesTests.java | 12 +++++++++++- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index b85c56c9ac424..abaad2733c8ce 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -391,9 +391,9 @@ private static boolean checkGroupByHaving(LogicalPlan p, Set localFailu Expressions.names(unsupported))); groupingFailures.add(a); return false; + } } } - } return true; } @@ -662,11 +662,21 @@ private static void checkGroupingFunctionTarget(GroupingFunction f, Set private static void checkFilterOnAggs(LogicalPlan p, Set localFailures, AttributeMap attributeRefs) { if (p instanceof Filter) { Filter filter = (Filter) p; - if ((filter.child() instanceof Aggregate) == false) { + LogicalPlan filterChild = filter.child(); + if (filterChild instanceof Aggregate == false) { filter.condition().forEachDown(Expression.class, e -> { if (Functions.isAggregate(attributeRefs.getOrDefault(e, e))) { - localFailures.add( - fail(e, "Cannot use WHERE filtering on aggregate function [{}], use HAVING instead", Expressions.name(e))); + if (filterChild instanceof Project) { + filter.condition().forEachDown(FieldAttribute.class, + fa -> localFailures.add( + fail(e, "[{}] field must appear in the GROUP BY clause or be used in an aggregate function", + Expressions.name(fa))) + ); + } else { + localFailures.add( + fail(e, "Cannot use WHERE filtering on aggregate function [{}], use HAVING instead", Expressions.name(e))); + + } } }); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 3ffc5756242ea..cd4caf9ac20b4 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -912,7 +912,17 @@ public void testIifWithDifferentResultAndDefaultValueDataTypes() { public void testAggsInWhere() { assertEquals("1:33: Cannot use WHERE filtering on aggregate function [MAX(int)], use HAVING instead", - error("SELECT MAX(int) FROM test WHERE MAX(int) > 10 GROUP BY bool")); + error("SELECT MAX(int) FROM test WHERE MAX(int) > 10 GROUP BY bool")); + } + + public void testHavingInAggs() { + assertEquals("1:29: [int] field must appear in the GROUP BY clause or be used in an aggregate function", + error("SELECT int FROM test HAVING MAX(int) = 0")); + + assertEquals("1:35: [int] field must appear in the GROUP BY clause or be used in an aggregate function", + error("SELECT int FROM test HAVING int = count(1)")); + // Note: "SELECT int FROM test HAVING int = 1" works, though it normally shouldn't; to correct this out, we'd need to qualify the + // Filter (WHERE vs HAVING); but this "extra flexibility" shouldn't be harmful atp. } public void testHistogramInFilter() { From e5c85d96871703b70717e52f9923efb5cf65fd93 Mon Sep 17 00:00:00 2001 From: Bogdan Pintea Date: Tue, 9 Feb 2021 21:51:01 +0100 Subject: [PATCH 2/2] Address review feedback - error message slight adjustment - adding one more test. --- .../xpack/sql/analysis/analyzer/Verifier.java | 9 ++++----- .../analyzer/VerifierErrorMessagesTests.java | 15 +++++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java index abaad2733c8ce..e449fb488aa99 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java @@ -668,13 +668,12 @@ private static void checkFilterOnAggs(LogicalPlan p, Set localFailures, if (Functions.isAggregate(attributeRefs.getOrDefault(e, e))) { if (filterChild instanceof Project) { filter.condition().forEachDown(FieldAttribute.class, - fa -> localFailures.add( - fail(e, "[{}] field must appear in the GROUP BY clause or be used in an aggregate function", - Expressions.name(fa))) + f -> localFailures.add(fail(e, "[{}] field must appear in the GROUP BY clause or in an aggregate function", + Expressions.name(f))) ); } else { - localFailures.add( - fail(e, "Cannot use WHERE filtering on aggregate function [{}], use HAVING instead", Expressions.name(e))); + localFailures.add(fail(e, "Cannot use WHERE filtering on aggregate function [{}], use HAVING instead", + Expressions.name(e))); } } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index cd4caf9ac20b4..bc2b8ff59b62a 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -916,13 +916,20 @@ public void testAggsInWhere() { } public void testHavingInAggs() { - assertEquals("1:29: [int] field must appear in the GROUP BY clause or be used in an aggregate function", + assertEquals("1:29: [int] field must appear in the GROUP BY clause or in an aggregate function", error("SELECT int FROM test HAVING MAX(int) = 0")); - assertEquals("1:35: [int] field must appear in the GROUP BY clause or be used in an aggregate function", + assertEquals("1:35: [int] field must appear in the GROUP BY clause or in an aggregate function", error("SELECT int FROM test HAVING int = count(1)")); - // Note: "SELECT int FROM test HAVING int = 1" works, though it normally shouldn't; to correct this out, we'd need to qualify the - // Filter (WHERE vs HAVING); but this "extra flexibility" shouldn't be harmful atp. + } + + public void testHavingAsWhere() { + // TODO: this query works, though it normally shouldn't; a check about it could only be enforced if the Filter would be qualified + // (WHERE vs HAVING). Otoh, this "extra flexibility" shouldn't be harmful atp. + accept("SELECT int FROM test HAVING int = 1"); + accept("SELECT int FROM test HAVING SIN(int) + 5 > 5.5"); + // HAVING's expression being AND'ed to WHERE's + accept("SELECT int FROM test WHERE int > 3 HAVING POWER(int, 2) < 100"); } public void testHistogramInFilter() {