SQL: Enhance Verifier to prevent aggregate or grouping functions from (…

…elastic#36799) Improve Verifier to prevent aggregate or grouping functions from being used in a WHERE clause. Fix elastic#36798
hub-cap · Dec 19, 2018 · 9584adf · 9584adf
1 parent d31eaf7
commit 9584adf
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 6 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
@@ -223,12 +223,13 @@ Collection<Failure> verify(LogicalPlan plan) {
                 validateInExpression(p, localFailures);
                 validateConditional(p, localFailures);
 
+                checkFilterOnAggs(p, localFailures);
+
                 if (!groupingFailures.contains(p)) {
                     checkGroupBy(p, localFailures, resolvedFunctions, groupingFailures);
                 }
 
                 checkForScoreInsideFunctions(p, localFailures);
-
                 checkNestedUsedInGroupByOrHaving(p, localFailures);
 
                 // everything checks out
@@ -370,7 +371,7 @@ private static boolean checkGroupByHaving(LogicalPlan p, Set<Failure> localFailu
                 if (!missing.isEmpty()) {
                     String plural = missing.size() > 1 ? "s" : StringUtils.EMPTY;
                     localFailures.add(
-                            fail(condition, "Cannot filter HAVING on non-aggregate" + plural + " %s; consider using WHERE instead",
+                            fail(condition, "Cannot use HAVING filter on non-aggregate" + plural + " %s; use WHERE instead",
                             Expressions.names(missing.keySet())));
                     groupingFailures.add(a);
                     return false;
@@ -542,6 +543,23 @@ private static boolean checkGroupMatch(Expression e, Node<?> source, List<Expres
         return false;
     }
 
+    private static void checkFilterOnAggs(LogicalPlan p, Set<Failure> localFailures) {
+        if (p instanceof Filter) {
+            Filter filter = (Filter) p;
+            if ((filter.child() instanceof Aggregate) == false) {
+                filter.condition().forEachDown(f -> {
+                    if (Functions.isAggregate(f) || Functions.isGrouping(f)) {
+                        String type = Functions.isAggregate(f) ? "aggregate" : "grouping";
+                        localFailures.add(fail(f,
+                                "Cannot use WHERE filtering on %s function [%s], use HAVING instead", type, Expressions.name(f)));
+                    }
+
+                }, Function.class);
+            }
+        }
+    }
+
+
     private static void checkForScoreInsideFunctions(LogicalPlan p, Set<Failure> localFailures) {
         // Make sure that SCORE is only used in "top level" functions
         p.forEachExpressions(e ->

diff --git a/...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/Functions.java b/...k/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/Functions.java
@@ -7,6 +7,7 @@
 
 import org.elasticsearch.xpack.sql.expression.Expression;
 import org.elasticsearch.xpack.sql.expression.function.aggregate.AggregateFunction;
+import org.elasticsearch.xpack.sql.expression.function.grouping.GroupingFunction;
 import org.elasticsearch.xpack.sql.plan.QueryPlan;
 
 import java.util.LinkedHashMap;
@@ -18,6 +19,10 @@ public static boolean isAggregate(Expression e) {
         return e instanceof AggregateFunction;
     }
 
+    public static boolean isGrouping(Expression e) {
+        return e instanceof GroupingFunction;
+    }
+
     public static Map<String, Function> collectFunctions(QueryPlan<?> plan) {
         Map<String, Function> resolvedFunctions = new LinkedHashMap<>();
         plan.forEachExpressionsDown(e -> {

diff --git a/...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/...c/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java
@@ -227,7 +227,7 @@ public void testGroupByOrderByScalarOverNonGrouped_WithHaving() {
     }
 
     public void testGroupByHavingNonGrouped() {
-        assertEquals("1:48: Cannot filter HAVING on non-aggregate [int]; consider using WHERE instead",
+        assertEquals("1:48: Cannot use HAVING filter on non-aggregate [int]; use WHERE instead",
                 error("SELECT AVG(int) FROM test GROUP BY text HAVING int > 10"));
     }
 
@@ -296,12 +296,12 @@ public void testGroupByOrderByScore() {
     }
 
     public void testHavingOnColumn() {
-        assertEquals("1:42: Cannot filter HAVING on non-aggregate [int]; consider using WHERE instead",
+        assertEquals("1:42: Cannot use HAVING filter on non-aggregate [int]; use WHERE instead",
                 error("SELECT int FROM test GROUP BY int HAVING int > 2"));
     }
 
     public void testHavingOnScalar() {
-        assertEquals("1:42: Cannot filter HAVING on non-aggregate [int]; consider using WHERE instead",
+        assertEquals("1:42: Cannot use HAVING filter on non-aggregate [int]; use WHERE instead",
                 error("SELECT int FROM test GROUP BY int HAVING 2 < ABS(int)"));
     }
 
@@ -474,4 +474,15 @@ public void testConditionalWithDifferentDataTypes_WhereClause() {
                 ": expected data type [KEYWORD], value provided is of type [INTEGER]",
             error("SELECT * FROM test WHERE " + arbirtraryArgsfunction + "(null, null, 'foo', 4) > 1"));
     }
-}
+
+    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"));
+    }
+
+    public void testHistogramInFilter() {
+        assertEquals("1:63: Cannot use WHERE filtering on grouping function [HISTOGRAM(date)], use HAVING instead",
+                error("SELECT HISTOGRAM(date, INTERVAL 1 MONTH) AS h FROM test WHERE "
+                        + "HISTOGRAM(date, INTERVAL 1 MONTH) > CAST('2000-01-01' AS DATE) GROUP BY h"));
+    }
+}