SQL: Fix result column names for arithmetic functions (elastic#33500)

Previously, when an arithmetic function got applied on a
table column in the `SELECT` clause, the name of the result
column contained weird characters used internally when
processing the SQL statement e.g.:

  SELECT CHAR(emp_no % 10000) FROM "test_emp"

returned:

  CHAR((emp_no{f}elastic#14) % 10000))

as the column name instead of:

  CHAR((emp_no) % 10000))

Also, fix an issue that causes a ClassCastException to be thrown
when using functions where both arguments are literals.

Closes elastic#31869
Closes elastic#33461

Author: Marios Trivyzas

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java

@@ -82,7 +82,13 @@ public static AttributeSet references(List<? extends Expression> exps) {
     }
 
     public static String name(Expression e) {
-        return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName();
+        if (e instanceof NamedExpression) {
+            return ((NamedExpression) e).name();
+        } else if (e instanceof Literal) {
+            return e.toString();
+        } else {
+            return e.nodeName();
+        }
     }
 
     public static List<String> names(Collection<? extends Expression> e) {
@@ -120,4 +126,4 @@ public static TypeResolution typeMustBeNumeric(Expression e) {
         return e.dataType().isNumeric()? TypeResolution.TYPE_RESOLVED : new TypeResolution(
                 "Argument required to be numeric ('" + Expressions.name(e) + "' of type '" + e.dataType().esType + "')");
     }
-}
+}

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

@@ -10,6 +10,7 @@
 import org.elasticsearch.xpack.sql.expression.Expression;
 import org.elasticsearch.xpack.sql.expression.Expressions;
 import org.elasticsearch.xpack.sql.expression.FieldAttribute;
+import org.elasticsearch.xpack.sql.expression.LiteralAttribute;
 import org.elasticsearch.xpack.sql.expression.function.Function;
 import org.elasticsearch.xpack.sql.expression.function.aggregate.AggregateFunctionAttribute;
 import org.elasticsearch.xpack.sql.expression.function.scalar.processor.definition.ProcessorDefinition;
@@ -68,6 +69,9 @@ protected ScriptTemplate asScript(Expression exp) {
             if (attr instanceof AggregateFunctionAttribute) {
                 return asScriptFrom((AggregateFunctionAttribute) attr);
             }
+            if (attr instanceof LiteralAttribute) {
+                return asScriptFrom((LiteralAttribute) attr);
+            }
             // fall-back to
             return asScriptFrom((FieldAttribute) attr);
         }
@@ -98,6 +102,12 @@ protected ScriptTemplate asScriptFrom(AggregateFunctionAttribute aggregate) {
                 aggregate.dataType());
     }
 
+    protected ScriptTemplate asScriptFrom(LiteralAttribute literal) {
+        return new ScriptTemplate(formatScript("{}"),
+            paramsBuilder().variable(literal.literal()).build(),
+            literal.dataType());
+    }
+
     protected String formatScript(String scriptTemplate) {
         return formatTemplate(scriptTemplate);
     }

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

@@ -6,6 +6,7 @@
 package org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic;
 
 import org.elasticsearch.xpack.sql.expression.Expression;
+import org.elasticsearch.xpack.sql.expression.Expressions;
 import org.elasticsearch.xpack.sql.expression.Literal;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.BinaryArithmeticProcessor.BinaryArithmeticOperation;
 import org.elasticsearch.xpack.sql.expression.function.scalar.math.BinaryNumericFunction;
@@ -65,7 +66,7 @@ protected ProcessorDefinition makeProcessorDefinition() {
     public String name() {
         StringBuilder sb = new StringBuilder();
         sb.append("(");
-        sb.append(left());
+        sb.append(Expressions.name(left()));
         if (!(left() instanceof Literal)) {
             sb.insert(1, "(");
             sb.append(")");
@@ -74,7 +75,7 @@ public String name() {
         sb.append(operation);
         sb.append(" ");
         int pos = sb.length();
-        sb.append(right());
+        sb.append(Expressions.name(right()));
         if (!(right() instanceof Literal)) {
             sb.insert(pos, "(");
             sb.append(")");
@@ -87,8 +88,4 @@ public String name() {
     public String toString() {
         return name() + "#" + functionId();
     }
-
-    protected boolean useParanthesis() {
-        return !(left() instanceof Literal) || !(right() instanceof Literal);
-    }
-}
+}

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/NamedExpressionTests.java

@@ -6,14 +6,18 @@
 package org.elasticsearch.xpack.sql.expression.function;
 
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.xpack.sql.expression.FieldAttribute;
 import org.elasticsearch.xpack.sql.expression.Literal;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Add;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Div;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Mod;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Mul;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Neg;
 import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Sub;
+import org.elasticsearch.xpack.sql.type.DataType;
+import org.elasticsearch.xpack.sql.type.EsField;
 
+import static java.util.Collections.emptyMap;
 import static org.elasticsearch.xpack.sql.tree.Location.EMPTY;
 
 public class NamedExpressionTests extends ESTestCase {
@@ -38,6 +42,12 @@ public void testArithmeticFunctionName() {
         assertEquals("-5", neg.name());
     }
 
+    public void testNameForArithmeticFunctionAppliedOnTableColumn() {
+        FieldAttribute fa = new FieldAttribute(EMPTY, "myField", new EsField("myESField", DataType.INTEGER, emptyMap(), true));
+        Add add = new Add(EMPTY, fa, l(10));
+        assertEquals("((myField) + 10)", add.name());
+    }
+
     private static Literal l(Object value) {
         return Literal.of(EMPTY, value);
     }

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvTestUtils.java

@@ -113,18 +113,18 @@ private static Tuple<String, String> extractColumnTypesAndStripCli(String expect
     }
 
     private static Tuple<String, String> extractColumnTypesFromHeader(String header) {
-        String[] columnTypes = Strings.delimitedListToStringArray(header, "|", " \t");
+        String[] columnTypes = Strings.tokenizeToStringArray(header, "|");
         StringBuilder types = new StringBuilder();
         StringBuilder columns = new StringBuilder();
         for (String column : columnTypes) {
-            String[] nameType = Strings.delimitedListToStringArray(column, ":");
+            String[] nameType = Strings.delimitedListToStringArray(column.trim(), ":");
             assertThat("If at least one column has a type associated with it, all columns should have types", nameType, arrayWithSize(2));
             if (types.length() > 0) {
                 types.append(",");
                 columns.append("|");
             }
-            columns.append(nameType[0]);
-            types.append(resolveColumnType(nameType[1]));
+            columns.append(nameType[0].trim());
+            types.append(resolveColumnType(nameType[1].trim()));
         }
         return new Tuple<>(columns.toString(), types.toString());
     }
@@ -206,4 +206,4 @@ public static class CsvTestCase {
         public String query;
         public String expectedResults;
     }
-}
+}

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java

@@ -176,8 +176,8 @@ private static void doAssertResultSetData(ResultSet expected, ResultSet actual,
                     Object expectedObject = expected.getObject(column);
                     Object actualObject = lenient ? actual.getObject(column, expectedColumnClass) : actual.getObject(column);
 
-                    String msg = format(Locale.ROOT, "Different result for column [" + metaData.getColumnName(column) + "], "
-                            + "entry [" + (count + 1) + "]");
+                    String msg = format(Locale.ROOT, "Different result for column [%s], entry [%d]",
+                        metaData.getColumnName(column), count + 1);
 
                     // handle nulls first
                     if (expectedObject == null || actualObject == null) {
@@ -230,4 +230,4 @@ private static int typeOf(int columnType, boolean lenient) {
 
         return columnType;
     }
-}
+}

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

@@ -407,3 +407,26 @@ SELECT CONCAT(CONCAT(SUBSTRING("first_name",1,LENGTH("first_name")-2),UCASE(LEFT
 ---------------+---------------------------------------------
 AlejandRo      |2
 ;
+
+
+checkColumnNameWithNestedArithmeticFunctionCallsOnTableColumn
+SELECT CHAR(emp_no % 10000) FROM "test_emp" WHERE emp_no > 10064 ORDER BY emp_no LIMIT 1;
+
+CHAR(((emp_no) % 10000)):s
+A
+;
+
+checkColumnNameWithComplexNestedArithmeticFunctionCallsOnTableColumn1
+SELECT CHAR(emp_no % (7000 + 3000)) FROM "test_emp" WHERE emp_no > 10065 ORDER BY emp_no LIMIT 1;
+
+CHAR(((emp_no) % ((7000 + 3000)))):s
+B
+;
+
+
+checkColumnNameWithComplexNestedArithmeticFunctionCallsOnTableColumn2
+SELECT CHAR((emp_no % (emp_no - 1 + 1)) + 67) FROM "test_emp" WHERE emp_no > 10066 ORDER BY emp_no LIMIT 1;
+
+CHAR(((((emp_no) % (((((emp_no) - 1)) + 1)))) + 67)):s
+C
+;

x-pack/qa/sql/src/main/resources/math.sql-spec

@@ -128,7 +128,9 @@ mathATan2
 // tag::atan2
 SELECT ATAN2(emp_no, emp_no) m, first_name FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;
 // end::atan2
-mathPower
 // tag::power
+mathPowerPositive
 SELECT POWER(emp_no, 2) m, first_name FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;
+mathPowerNegative
+SELECT POWER(salary, -1) m, first_name FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;
 // end::power