Use one method for the optional math functions

matriv · Mar 20, 2019 · b35090e · b35090e
1 parent 0c2f559
commit b35090e
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 19 deletions.
diff --git a/...lasticsearch/xpack/sql/expression/function/scalar/math/BinaryOptionalNumericFunction.java b/...lasticsearch/xpack/sql/expression/function/scalar/math/BinaryOptionalNumericFunction.java
@@ -9,6 +9,7 @@
 import org.elasticsearch.xpack.sql.expression.Expression;
 import org.elasticsearch.xpack.sql.expression.Expressions;
 import org.elasticsearch.xpack.sql.expression.Expressions.ParamOrdinal;
+import org.elasticsearch.xpack.sql.expression.Literal;
 import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
 import org.elasticsearch.xpack.sql.expression.function.scalar.math.BinaryOptionalMathProcessor.BinaryOptionalMathOperation;
 import org.elasticsearch.xpack.sql.expression.gen.pipeline.Pipe;
@@ -88,20 +89,12 @@ public Expression replaceChildren(List<Expression> newChildren) {
     @Override
     public ScriptTemplate asScript() {
         ScriptTemplate leftScript = asScript(left);
-        ScriptTemplate rightScript = right == null ? null : asScript(right);
+        ScriptTemplate rightScript = right == null ? asScript(Literal.NULL) : asScript(right);
 
         return asScriptFrom(leftScript, rightScript);
     }
 
     private ScriptTemplate asScriptFrom(ScriptTemplate leftScript, ScriptTemplate rightScript) {
-        if (right == null) {
-            return new ScriptTemplate(format(Locale.ROOT, formatTemplate("{sql}.%s(%s)"),
-                    operation().name().toLowerCase(Locale.ROOT),
-                    leftScript.template()),
-                    paramsBuilder()
-                        .script(leftScript.params())
-                        .build(), dataType());
-        }
         return new ScriptTemplate(format(Locale.ROOT, formatTemplate("{sql}.%s(%s,%s)"),
                 operation().name().toLowerCase(Locale.ROOT),
                 leftScript.template(),

diff --git a/.../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java b/.../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
@@ -197,18 +197,10 @@ public static Object sub(Object left, Object right) {
         return BinaryArithmeticOperation.SUB.apply(left, right);
     }
 
-    public static Number round(Number v) {
-        return round(v, null);
-    }
-
     public static Number round(Number v, Number s) {
         return BinaryOptionalMathOperation.ROUND.apply(v, s);
     }
 
-    public static Number truncate(Number v) {
-        return truncate(v, null);
-    }
-
     public static Number truncate(Number v, Number s) {
         return BinaryOptionalMathOperation.TRUNCATE.apply(v, s);
     }

diff --git a/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt b/x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt
@@ -67,9 +67,7 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS
   Number atan2(Number, Number)
   Number neg(Number)
   Number power(Number, Number)
-  Number round(Number)
   Number round(Number, Number)
-  Number truncate(Number)
   Number truncate(Number, Number)
 
   Double abs(Number)

diff --git a/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
@@ -420,6 +420,27 @@ public void testTranslateMathFunction_HavingClause_Painless() {
         assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=max(int){a->"));
         assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]"));
     }
+
+    public void testTranslateRoundWithOneParameter() {
+        LogicalPlan p = plan("SELECT ROUND(YEAR(date)) FROM test GROUP BY ROUND(YEAR(date))");
+
+        assertTrue(p instanceof Aggregate);
+        assertEquals(1, ((Aggregate) p).groupings().size());
+        assertEquals(1, ((Aggregate) p).aggregates().size());
+        assertTrue(((Aggregate) p).groupings().get(0) instanceof Round);
+        assertTrue(((Aggregate) p).aggregates().get(0) instanceof Round);
+
+        Round groupingRound = (Round) ((Aggregate) p).groupings().get(0);
+        assertEquals(1, groupingRound.children().size());
+
+        QueryTranslator.GroupingContext groupingContext = QueryTranslator.groupBy(((Aggregate) p).groupings());
+        assertNotNull(groupingContext);
+        ScriptTemplate scriptTemplate = groupingContext.tail.script();
+        assertEquals("InternalSqlScriptUtils.round(InternalSqlScriptUtils.dateTimeChrono(InternalSqlScriptUtils.docValue(doc,params.v0), "
+                + "params.v1, params.v2),params.v3)",
+            scriptTemplate.toString());
+        assertEquals("[{v=date}, {v=Z}, {v=YEAR}, {v=null}]", scriptTemplate.params().toString());
+    }
 
     public void testTranslateRoundWithTwoParameters() {
         LogicalPlan p = plan("SELECT ROUND(YEAR(date), -2) FROM test GROUP BY ROUND(YEAR(date), -2)");