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 386be1eadcc..47f68a640c7 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 @@ -223,12 +223,13 @@ public final class Verifier { 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 @@ public final class Verifier { 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 @@ public final class Verifier { return false; } + private static void checkFilterOnAggs(LogicalPlan p, Set 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 localFailures) { // Make sure that SCORE is only used in "top level" functions p.forEachExpressions(e -> diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/Functions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/Functions.java index 7f5465b7413..46ca0ea91b4 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/Functions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/Functions.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.expression.function; 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 abstract class Functions { return e instanceof AggregateFunction; } + public static boolean isGrouping(Expression e) { + return e instanceof GroupingFunction; + } + public static Map collectFunctions(QueryPlan plan) { Map resolvedFunctions = new LinkedHashMap<>(); plan.forEachExpressionsDown(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 fcb46d7f8d4..a3fd459bf3c 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 @@ -227,7 +227,7 @@ public class VerifierErrorMessagesTests extends ESTestCase { } 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 class VerifierErrorMessagesTests extends ESTestCase { } 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 class VerifierErrorMessagesTests extends ESTestCase { ": 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")); + } +} \ No newline at end of file