From 93744ee8d36e235d23b410c25a9d3c2b4f231089 Mon Sep 17 00:00:00 2001 From: New Author Date: Thu, 6 Sep 2018 16:57:06 +0200 Subject: [PATCH 1/5] SQL: Fix result column names for arithmetic funcs Previously, When an arithmetic function is applied on a table column in the `SELECT` clause the name of the result column name 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)) Fixes: https://github.com/elastic/elasticsearch/issues/31869 --- .../scalar/arithmetic/ArithmeticFunction.java | 15 ++++++++++----- .../expression/function/NamedExpressionTests.java | 10 ++++++++++ .../xpack/qa/sql/jdbc/CsvTestUtils.java | 10 +++++----- .../xpack/qa/sql/jdbc/JdbcAssert.java | 6 +++--- .../qa/sql/src/main/resources/functions.csv-spec | 8 ++++++++ 5 files changed, 36 insertions(+), 13 deletions(-) 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..8e4a49507cfe2 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.FieldAttribute; 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()); + appendFunctionArg(left(), sb); 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()); + appendFunctionArg(right(), sb); if (!(right() instanceof Literal)) { sb.insert(pos, "("); sb.append(")"); @@ -88,7 +89,11 @@ public String toString() { return name() + "#" + functionId(); } - protected boolean useParanthesis() { - return !(left() instanceof Literal) || !(right() instanceof Literal); + private void appendFunctionArg(Expression arg, StringBuilder sb) { + if (arg instanceof FieldAttribute) { + sb.append(((FieldAttribute) arg).name()); + } else { + sb.append(arg); + } } -} \ 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..b2144f4c46463 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,11 @@ 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 +; \ No newline at end of file From 032686cf420e756387df4c796067b41e7886fa0a Mon Sep 17 00:00:00 2001 From: New Author Date: Fri, 7 Sep 2018 13:06:07 +0200 Subject: [PATCH 2/5] fixup! SQL: Fix result column names for arithmetic funcs --- .../xpack/sql/expression/Expressions.java | 10 ++++++++-- .../scalar/arithmetic/ArithmeticFunction.java | 13 +++---------- 2 files changed, 11 insertions(+), 12 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/arithmetic/ArithmeticFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/arithmetic/ArithmeticFunction.java index 8e4a49507cfe2..0bef0c1977edc 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.FieldAttribute; import org.elasticsearch.xpack.sql.expression.Literal; import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.BinaryArithmeticProcessor.BinaryArithmeticOperation; @@ -66,7 +67,7 @@ protected ProcessorDefinition makeProcessorDefinition() { public String name() { StringBuilder sb = new StringBuilder(); sb.append("("); - appendFunctionArg(left(), sb); + sb.append(Expressions.name(left())); if (!(left() instanceof Literal)) { sb.insert(1, "("); sb.append(")"); @@ -75,7 +76,7 @@ public String name() { sb.append(operation); sb.append(" "); int pos = sb.length(); - appendFunctionArg(right(), sb); + sb.append(Expressions.name(right())); if (!(right() instanceof Literal)) { sb.insert(pos, "("); sb.append(")"); @@ -88,12 +89,4 @@ public String name() { public String toString() { return name() + "#" + functionId(); } - - private void appendFunctionArg(Expression arg, StringBuilder sb) { - if (arg instanceof FieldAttribute) { - sb.append(((FieldAttribute) arg).name()); - } else { - sb.append(arg); - } - } } From c413094b7074e1de905543ab04cc08fe0261c01d Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 7 Sep 2018 15:56:08 +0200 Subject: [PATCH 3/5] fixup! fixup! SQL: Fix result column names for arithmetic funcs --- .../function/scalar/arithmetic/ArithmeticFunction.java | 1 - 1 file changed, 1 deletion(-) 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 0bef0c1977edc..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 @@ -7,7 +7,6 @@ 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.Literal; import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.BinaryArithmeticProcessor.BinaryArithmeticOperation; import org.elasticsearch.xpack.sql.expression.function.scalar.math.BinaryNumericFunction; From 5b046e25e4f5b5db929e139978d365c016da89f7 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 7 Sep 2018 17:48:30 +0200 Subject: [PATCH 4/5] SQL: Add more tests for complex function calls Add more integ tests for complex functions calls in `SELECT` clause, to test both the correct column names returned but also the evaluation itself. Fix an issue that causes a ClassCastException to be thrown when using functions where both arguments are literals. --- .../function/scalar/ScalarFunction.java | 10 ++++++++++ .../sql/src/main/resources/functions.csv-spec | 17 ++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) 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/qa/sql/src/main/resources/functions.csv-spec b/x-pack/qa/sql/src/main/resources/functions.csv-spec index b2144f4c46463..3622cfe043381 100644 --- a/x-pack/qa/sql/src/main/resources/functions.csv-spec +++ b/x-pack/qa/sql/src/main/resources/functions.csv-spec @@ -414,4 +414,19 @@ SELECT CHAR(emp_no % 10000) FROM "test_emp" WHERE emp_no > 10064 ORDER BY emp_no CHAR(((emp_no) % 10000)):s A -; \ No newline at end of file +; + +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 +; From 86926167cce6eb75acc4921cf2f12f1976091ec7 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Mon, 10 Sep 2018 11:23:07 +0200 Subject: [PATCH 5/5] !fixup Add test with negative literal. https://github.com/elastic/elasticsearch/issues/33461 --- x-pack/qa/sql/src/main/resources/math.sql-spec | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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