From 06d743195091c96a45fc410759b788db60437f35 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 25 Oct 2018 22:15:28 +0200 Subject: [PATCH] SQL: Return error with ORDER BY on non-grouped. (#34855) Previously, for some queries the validation for ORDER BY fields didn't kick in since a HAVING close or an ORDER BY with scalar function would add `Filter` and `Project` plans between the `OrderBy` and the `Aggregate`. Now the LogicalPlan tree beneath `OrderBy` is traversed and the ORDER BY fields are properly verified. Fixes: #34590 --- .../xpack/sql/analysis/analyzer/Verifier.java | 28 +++++++++++++------ .../analyzer/VerifierErrorMessagesTests.java | 10 +++++++ 2 files changed, 30 insertions(+), 8 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 32d57175114d8..c8834240c6ceb 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 @@ -25,6 +25,7 @@ import org.elasticsearch.xpack.sql.plan.logical.Filter; import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.sql.plan.logical.OrderBy; +import org.elasticsearch.xpack.sql.plan.logical.Project; import org.elasticsearch.xpack.sql.tree.Node; import org.elasticsearch.xpack.sql.type.DataType; import org.elasticsearch.xpack.sql.util.StringUtils; @@ -238,8 +239,17 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur Set groupingFailures, Map functions) { if (p instanceof OrderBy) { OrderBy o = (OrderBy) p; - if (o.child() instanceof Aggregate) { - Aggregate a = (Aggregate) o.child(); + LogicalPlan child = o.child(); + + if (child instanceof Project) { + child = ((Project) child).child(); + } + if (child instanceof Filter) { + child = ((Filter) child).child(); + } + + if (child instanceof Aggregate) { + Aggregate a = (Aggregate) child; Map> missing = new LinkedHashMap<>(); o.order().forEach(oe -> { @@ -253,7 +263,7 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur // take aliases declared inside the aggregates which point to the grouping (but are not included in there) // to correlate them to the order List groupingAndMatchingAggregatesAliases = new ArrayList<>(a.groupings()); - + a.aggregates().forEach(as -> { if (as instanceof Alias) { Alias al = (Alias) as; @@ -262,10 +272,13 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur } } }); - - // make sure to compare attributes directly - if (Expressions.anyMatch(groupingAndMatchingAggregatesAliases, - g -> e.semanticEquals(e instanceof Attribute ? Expressions.attribute(g) : g))) { + + // Make sure you can apply functions on top of the grouped by expressions in the ORDER BY: + // e.g.: if "GROUP BY f2(f1(field))" you can "ORDER BY f4(f3(f2(f1(field))))" + // + // Also, make sure to compare attributes directly + if (e.anyMatch(expression -> Expressions.anyMatch(groupingAndMatchingAggregatesAliases, + g -> expression.semanticEquals(expression instanceof Attribute ? Expressions.attribute(g) : g)))) { return; } @@ -288,7 +301,6 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set localFailur return true; } - private static boolean checkGroupByHaving(LogicalPlan p, Set localFailures, Set groupingFailures, Map functions) { if (p instanceof Filter) { 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 bc4f6a9f95cbd..e69b694968a25 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 @@ -118,6 +118,11 @@ public void testGroupByOrderByNonGrouped() { verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY bool")); } + public void testGroupByOrderByNonGrouped_WithHaving() { + assertEquals("1:71: Cannot order by non-grouped column [bool], expected [text]", + verify("SELECT MAX(int) FROM test GROUP BY text HAVING MAX(int) > 10 ORDER BY bool")); + } + public void testGroupByOrderByAliasedInSelectAllowed() { LogicalPlan lp = accepted("SELECT text t FROM test GROUP BY text ORDER BY t"); assertNotNull(lp); @@ -128,6 +133,11 @@ public void testGroupByOrderByScalarOverNonGrouped() { verify("SELECT MAX(int) FROM test GROUP BY text ORDER BY YEAR(date)")); } + public void testGroupByOrderByScalarOverNonGrouped_WithHaving() { + assertEquals("1:71: Cannot order by non-grouped column [YEAR(date [UTC])], expected [text]", + verify("SELECT MAX(int) FROM test GROUP BY text HAVING MAX(int) > 10 ORDER BY YEAR(date)")); + } + public void testGroupByHavingNonGrouped() { assertEquals("1:48: Cannot filter by non-grouped column [int], expected [text]", verify("SELECT AVG(int) FROM test GROUP BY text HAVING int > 10"));