From d28c23a61744477e118096fd18772cdbcbd08d8f Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 10 Sep 2018 22:02:11 +0200 Subject: [PATCH] SQL: Fix result column names for arithmetic functions (#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}#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 #31869 Closes #33461 --- .../xpack/sql/expression/Expressions.java | 10 ++++++-- .../function/scalar/ScalarFunction.java | 10 ++++++++ .../scalar/arithmetic/ArithmeticFunction.java | 11 ++++----- .../function/NamedExpressionTests.java | 10 ++++++++ .../xpack/qa/sql/jdbc/CsvTestUtils.java | 10 ++++---- .../xpack/qa/sql/jdbc/JdbcAssert.java | 6 ++--- .../sql/src/main/resources/functions.csv-spec | 23 +++++++++++++++++++ .../qa/sql/src/main/resources/math.sql-spec | 4 +++- 8 files changed, 66 insertions(+), 18 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java index 5851e99131435..8ee34e32a552b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java @@ -82,7 +82,13 @@ public static AttributeSet references(List 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 names(Collection 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 + "')"); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/ScalarFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/ScalarFunction.java index 8462ee293cc48..309ee4e8e8638 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/ScalarFunction.java +++ b/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); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java index 5715e19963cbc..e95fec863971b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java +++ b/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); - } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/NamedExpressionTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/NamedExpressionTests.java index 79f0e970b1eba..3692e5e4752af 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/NamedExpressionTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/NamedExpressionTests.java @@ -6,6 +6,7 @@ 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; @@ -13,7 +14,10 @@ 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); } diff --git a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvTestUtils.java b/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvTestUtils.java index a5e8b549bce8f..856629f8d9188 100644 --- a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvTestUtils.java +++ b/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/CsvTestUtils.java @@ -113,18 +113,18 @@ private static Tuple extractColumnTypesAndStripCli(String expect } private static Tuple 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; } -} \ No newline at end of file +} diff --git a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java b/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java index 47f531ebd1f9b..133006c66a820 100644 --- a/x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/JdbcAssert.java +++ b/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; } -} \ No newline at end of file +} diff --git a/x-pack/qa/sql/src/main/resources/functions.csv-spec b/x-pack/qa/sql/src/main/resources/functions.csv-spec index 1a610aec04861..3622cfe043381 100644 --- a/x-pack/qa/sql/src/main/resources/functions.csv-spec +++ b/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 +; diff --git a/x-pack/qa/sql/src/main/resources/math.sql-spec b/x-pack/qa/sql/src/main/resources/math.sql-spec index e38de2aa6bcbf..6452d2a3ac0a6 100644 --- a/x-pack/qa/sql/src/main/resources/math.sql-spec +++ b/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