SQL: Fix ORDER BY YEAR() function

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/ScalarFunction.java

@@ -39,17 +39,15 @@ protected ScalarFunction(Source source, List<Expression> fields) {
         super(source, fields);
     }
 
-    // used if the function is monotonic and thus does not have to be computed for ordering purposes
-    // null means the script needs to be used; expression means the field/expression to be used instead
+    // Every function needs to be translated to a script
+    // which will be used for the ordering
     public Expression orderBy() {
         return null;
     }
 
-
     //
     // Script generation
     //
-
     public ScriptTemplate asScript(Expression exp) {
         if (exp.foldable()) {
             return scriptWithFoldable(exp);
@@ -73,7 +71,6 @@ public ScriptTemplate asScript(Expression exp) {
         throw new QlIllegalArgumentException("Cannot evaluate script for expression {}", exp);
     }
 
-
     protected ScriptTemplate scriptWithFoldable(Expression foldable) {
         Object fold = foldable.fold();
 
@@ -144,4 +141,4 @@ protected String processScript(String script) {
     protected String formatTemplate(String template) {
         return Scripts.formatTemplate(template);
     }
-}
+}

x-pack/plugin/sql/qa/src/main/resources/datetime.sql-spec

@@ -124,6 +124,12 @@ SELECT first_name FROM test_emp ORDER BY NOW(), first_name NULLS LAST LIMIT 5;
 groupByCurrentTimestamp
 SELECT MAX(salary) AS max FROM test_emp GROUP BY NOW();
 
+//
+// ORDER BY
+//
+orderByYear
+SELECT YEAR(birth_date) as year, emp_no FROM test_emp ORDER BY year NULLS FIRST, emp_no ASC;
+
 // ES will consider a TIME in UTC, if no other indication is given and H2 doesn't consider the timezone for times, so no TZSync'ing needed.
 hourFromStringTime
 SELECT HOUR(CAST('18:09:03' AS TIME)) AS result;

x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec

@@ -939,7 +939,7 @@ SELECT -2 * INTERVAL '3' YEARS AS result;
 
 ///////////////////////////////
 //
-// Order by
+// Order By
 //
 ///////////////////////////////
 

x-pack/plugin/sql/qa/src/main/resources/nested.csv-spec

@@ -102,27 +102,15 @@ Mayuko     | 12
 ;
 
 selectWithScalarOnNested
-SELECT first_name f, last_name l, YEAR(dep.from_date) start FROM test_emp WHERE dep.dep_name = 'Production' AND languages > 1 ORDER BY start LIMIT 5;
+SELECT first_name f, last_name l, YEAR(dep.from_date) start FROM test_emp WHERE dep.dep_name = 'Production' AND languages > 1 ORDER BY dep.from_date LIMIT 5;
 
 f:s                  | l:s           | start:i
 
-Sreekrishna          |Servieres      |1985           
-Zhongwei             |Rosen          |1986           
-Chirstian            |Koblick        |1986           
-null                 |Chappelet      |1988           
-Zvonko               |Nyanchama      |1989     
-;
-
-selectWithScalarOnNestedWithoutProjection
-SELECT first_name f, last_name l FROM test_emp WHERE dep.dep_name = 'Production' AND languages > 1 ORDER BY YEAR(dep.from_date) LIMIT 5;
-
-f:s                  | l:s
-
-Sreekrishna          |Servieres      
-Zhongwei             |Rosen          
-Chirstian            |Koblick        
-null                 |Chappelet      
-Zvonko               |Nyanchama  
+Sreekrishna          |Servieres      |1985
+Zhongwei             |Rosen          |1986
+Chirstian            |Koblick        |1986
+null                 |Chappelet      |1988
+Zvonko               |Nyanchama      |1989
 ;
 
 //

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

@@ -253,7 +253,7 @@ Collection<Failure> verify(LogicalPlan plan) {
                 }
 
                 checkForScoreInsideFunctions(p, localFailures);
-                checkNestedUsedInGroupByOrHaving(p, localFailures);
+                checkNestedUsedInGroupByOrHavingOrOrderBy(p, localFailures, attributeRefs);
                 checkForGeoFunctionsOnDocValues(p, localFailures);
                 checkPivot(p, localFailures, attributeRefs);
 
@@ -722,16 +722,19 @@ private static void checkForScoreInsideFunctions(LogicalPlan p, Set<Failure> loc
                 Function.class));
     }
 
-    private static void checkNestedUsedInGroupByOrHaving(LogicalPlan p, Set<Failure> localFailures) {
+    private static void checkNestedUsedInGroupByOrHavingOrOrderBy(LogicalPlan p, Set<Failure> localFailures,
+                                                                  AttributeMap<Expression> attributeRefs) {
         List<FieldAttribute> nested = new ArrayList<>();
-        Consumer<FieldAttribute> match = fa -> {
+        Consumer<FieldAttribute> match = (fa) -> {
             if (fa.isNested()) {
                 nested.add(fa);
             }
         };
 
         // nested fields shouldn't be used in aggregates or having (yet)
-        p.forEachDown(a -> a.groupings().forEach(agg -> agg.forEachUp(match, FieldAttribute.class)), Aggregate.class);
+        p.forEachDown(a -> a.groupings().forEach(agg -> agg.forEachUp(e ->
+                attributeRefs.getOrDefault(e, e).forEachUp(match, FieldAttribute.class)
+        )), Aggregate.class);
 
         if (!nested.isEmpty()) {
             localFailures.add(
@@ -740,15 +743,26 @@ private static void checkNestedUsedInGroupByOrHaving(LogicalPlan p, Set<Failure>
         }
 
         // check in having
-        p.forEachDown(f -> {
-            if (f.child() instanceof Aggregate) {
-                f.condition().forEachUp(match, FieldAttribute.class);
-            }
-        }, Filter.class);
+        p.forEachDown(f -> f.forEachDown(a -> f.condition().forEachUp(e ->
+                        attributeRefs.getOrDefault(e, e).forEachUp(match, FieldAttribute.class)),
+                Aggregate.class), Filter.class);
 
         if (!nested.isEmpty()) {
             localFailures.add(
                     fail(nested.get(0), "HAVING isn't (yet) compatible with nested fields " + new AttributeSet(nested).names()));
+            nested.clear();
+        }
+
+        // check in order by (scalars not allowed)
+        p.forEachDown(ob -> ob.order().forEach(o -> o.forEachUp(e ->
+                attributeRefs.getOrDefault(e, e).forEachUp(f -> f.arguments().forEach(
+                        arg -> arg.forEachUp(match, FieldAttribute.class)), ScalarFunction.class)
+        )), OrderBy.class);
+
+        if (!nested.isEmpty()) {
+            localFailures.add(
+                    fail(nested.get(0), "ORDER BY isn't (yet) compatible with scalar functions on nested fields " +
+                            new AttributeSet(nested).names()));
         }
     }
 

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/Year.java

@@ -39,11 +39,6 @@ public String dateTimeFormat() {
         return "year";
     }
 
-    @Override
-    public Expression orderBy() {
-        return field();
-    }
-
     @Override
     public String calendarInterval() {
         return YEAR_INTERVAL;

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java

@@ -470,11 +470,36 @@ public void testGroupByOnInexact() {
     public void testGroupByOnNested() {
         assertEquals("1:38: Grouping isn't (yet) compatible with nested fields [dep.dep_id]",
                 error("SELECT dep.dep_id FROM test GROUP BY dep.dep_id"));
+        assertEquals("1:8: Grouping isn't (yet) compatible with nested fields [dep.dep_id]",
+                error("SELECT dep.dep_id AS a FROM test GROUP BY a"));
+        assertEquals("1:8: Grouping isn't (yet) compatible with nested fields [dep.dep_id]",
+                error("SELECT dep.dep_id AS a FROM test GROUP BY 1"));
+        assertEquals("1:8: Grouping isn't (yet) compatible with nested fields [dep.dep_id, dep.start_date]",
+                error("SELECT dep.dep_id AS a, dep.start_date AS b FROM test GROUP BY 1, 2"));
+        assertEquals("1:8: Grouping isn't (yet) compatible with nested fields [dep.dep_id, dep.start_date]",
+                error("SELECT dep.dep_id AS a, dep.start_date AS b FROM test GROUP BY a, b"));
     }
 
     public void testHavingOnNested() {
         assertEquals("1:51: HAVING isn't (yet) compatible with nested fields [dep.start_date]",
                 error("SELECT int FROM test GROUP BY int HAVING AVG(YEAR(dep.start_date)) > 1980"));
+        assertEquals("1:22: HAVING isn't (yet) compatible with nested fields [dep.start_date]",
+                error("SELECT int, AVG(YEAR(dep.start_date)) AS average FROM test GROUP BY int HAVING average > 1980"));
+        assertEquals("1:22: HAVING isn't (yet) compatible with nested fields [dep.start_date, dep.end_date]",
+                error("SELECT int, AVG(YEAR(dep.start_date)) AS a, MAX(MONTH(dep.end_date)) AS b " +
+                      "FROM test GROUP BY int " +
+                      "HAVING a > 1980 AND b < 10"));
+    }
+
+    public void testOrderByOnNested() {
+        assertEquals("1:36: ORDER BY isn't (yet) compatible with scalar functions on nested fields [dep.start_date]",
+                error("SELECT int FROM test ORDER BY YEAR(dep.start_date) + 10"));
+        assertEquals("1:13: ORDER BY isn't (yet) compatible with scalar functions on nested fields [dep.start_date]",
+                error("SELECT YEAR(dep.start_date) + 10  FROM test ORDER BY 1"));
+        assertEquals("1:13: ORDER BY isn't (yet) compatible with scalar functions on nested fields " +
+                        "[dep.start_date, dep.end_date]",
+                error("SELECT YEAR(dep.start_date) + 10 AS a, MONTH(dep.end_date) - 10 as b FROM test ORDER BY 1, 2"));
+        accept("SELECT int FROM test ORDER BY dep.start_date, dep.end_date");
     }
 
     public void testGroupByScalarFunctionWithAggOnTarget() {