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 6f8be61b463fd..4915a25a55bc7 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 @@ -213,10 +213,11 @@ static Collection verify(LogicalPlan plan) { * Check validity of Aggregate/GroupBy. * This rule is needed for multiple reasons: * 1. a user might specify an invalid aggregate (SELECT foo GROUP BY bar) - * 2. the order/having might contain a non-grouped attribute. This is typically + * 2. the ORDER BY/HAVING might contain a non-grouped attribute. This is typically * caught by the Analyzer however if wrapped in a function (ABS()) it gets resolved * (because the expression gets resolved little by little without being pushed down, * without the Analyzer modifying anything. + * 2a. HAVING also requires an Aggregate function * 3. composite agg (used for GROUP BY) allows ordering only on the group keys */ private static boolean checkGroupBy(LogicalPlan p, Set localFailures, @@ -244,7 +245,7 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur } // make sure to compare attributes directly - if (Expressions.anyMatch(a.groupings(), + if (Expressions.anyMatch(a.groupings(), g -> e.semanticEquals(e instanceof Attribute ? Expressions.attribute(g) : g))) { return; } @@ -278,13 +279,14 @@ private static boolean checkGroupByHaving(LogicalPlan p, Set localFailu Map> missing = new LinkedHashMap<>(); Expression condition = f.condition(); - condition.collectFirstChildren(c -> checkGroupMatch(c, condition, a.groupings(), missing, functions)); + // variation of checkGroupMatch customized for HAVING, which requires just aggregations + condition.collectFirstChildren(c -> checkGroupByHavingHasOnlyAggs(c, condition, missing, functions)); if (!missing.isEmpty()) { String plural = missing.size() > 1 ? "s" : StringUtils.EMPTY; - localFailures.add(fail(condition, "Cannot filter by non-grouped column" + plural + " %s, expected %s", - Expressions.names(missing.keySet()), - Expressions.names(a.groupings()))); + localFailures.add( + fail(condition, "Cannot filter HAVING on non-aggregate" + plural + " %s; consider using WHERE instead", + Expressions.names(missing.keySet()))); groupingFailures.add(a); return false; } @@ -294,6 +296,57 @@ private static boolean checkGroupByHaving(LogicalPlan p, Set localFailu } + private static boolean checkGroupByHavingHasOnlyAggs(Expression e, Node source, + Map> missing, Map functions) { + + // resolve FunctionAttribute to backing functions + if (e instanceof FunctionAttribute) { + FunctionAttribute fa = (FunctionAttribute) e; + Function function = functions.get(fa.functionId()); + // TODO: this should be handled by a different rule + if (function == null) { + return false; + } + e = function; + } + + // scalar functions can be a binary tree + // first test the function against the grouping + // and if that fails, start unpacking hoping to find matches + if (e instanceof ScalarFunction) { + ScalarFunction sf = (ScalarFunction) e; + + // unwrap function to find the base + for (Expression arg : sf.arguments()) { + arg.collectFirstChildren(c -> checkGroupByHavingHasOnlyAggs(c, source, missing, functions)); + } + return true; + + } else if (e instanceof Score) { + // Score can't be used for having + missing.put(e, source); + return true; + } + + // skip literals / foldable + if (e.foldable()) { + return true; + } + // skip aggs (allowed to refer to non-group columns) + if (Functions.isAggregate(e)) { + return true; + } + + // left without leaves which have to match; that's a failure since everything should be based on an agg + if (e instanceof Attribute) { + missing.put(e, source); + return true; + } + + return false; + } + + // check whether plain columns specified in an agg are mentioned in the group-by private static boolean checkGroupByAgg(LogicalPlan p, Set localFailures, Set groupingFailures, Map functions) { 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 60875e0194a0c..dce665a97e95d 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 @@ -159,4 +159,14 @@ public void testGroupByOrderByScore() { assertEquals("1:44: Cannot order by non-grouped column [SCORE()], expected [int]", verify("SELECT int FROM test GROUP BY int ORDER BY SCORE()")); } + + public void testHavingOnColumn() { + assertEquals("1:42: Cannot filter HAVING on non-aggregate [int]; consider using WHERE instead", + verify("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", + verify("SELECT int FROM test GROUP BY int HAVING 2 < ABS(int)")); + } } \ No newline at end of file