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

Author: Marios Trivyzas

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<Failure> localFailur
             Set<LogicalPlan> groupingFailures, Map<String, Function> 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<Expression, Node<?>> missing = new LinkedHashMap<>();
                 o.order().forEach(oe -> {
@@ -253,7 +263,7 @@ private static boolean checkGroupByOrder(LogicalPlan p, Set<Failure> 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<Expression> 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<Failure> 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<Failure> localFailur
         return true;
     }
 
-
     private static boolean checkGroupByHaving(LogicalPlan p, Set<Failure> localFailures,
             Set<LogicalPlan> groupingFailures, Map<String, Function> functions) {
         if (p instanceof Filter) {

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"));