From db8f07c665d4cf4f10f0ff221572c8ebda6e50b7 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 14 Dec 2018 14:51:12 +0200 Subject: [PATCH] SQL: Fix translation to painless for conditionals (#36636) Add missing `formatTemplate()` for conditional functions which resulted in incomplete painless script. Moreover the specific return type of Object in the painless signatures resulted in casting exceptions when conditional functions are used in the ORDER BY. Fixes: #36631 --- .../sql/qa/src/main/resources/null.sql-spec | 12 ++++++++ .../ArbitraryConditionalFunction.java | 2 +- .../predicate/conditional/NullIf.java | 2 +- .../xpack/sql/plugin/sql_whitelist.txt | 8 ++--- .../sql/planner/QueryTranslatorTests.java | 29 +++++++++++++++++++ 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/null.sql-spec b/x-pack/plugin/sql/qa/src/main/resources/null.sql-spec index 8da5d8c1e8b..d5a21262ca5 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/null.sql-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/null.sql-spec @@ -11,6 +11,9 @@ SELECT COALESCE(null, ABS(MAX(emp_no)) + 1, 123) AS c FROM test_emp GROUP BY lan coalesceWhere SELECT COALESCE(null, ABS(emp_no) + 1, 123) AS c FROM test_emp WHERE COALESCE(null, ABS(emp_no) + 1, 123, 321) > 100 ORDER BY emp_no NULLS FIRST LIMIT 5; +coalesceOrderBy +SELECT COALESCE(null, ABS(emp_no) + 1, 123) AS c FROM test_emp ORDER BY c NULLS FIRST LIMIT 5; + ifNullField SELECT IFNULL(null, ABS(emp_no) + 1) AS "ifnull" FROM test_emp ORDER BY emp_no LIMIT 5; @@ -23,6 +26,9 @@ SELECT NULLIF(10002, ABS(emp_no) + 1) AS c, emp_no FROM test_emp WHERE NULLIF(10 nullIfHaving SELECT NULLIF(10030, ABS(MAX(emp_no)) + 1) AS nif FROM test_emp GROUP BY languages HAVING nif IS NOT NULL ORDER BY languages; +nullIfOrderBy +SELECT NULLIF(10030, ABS(emp_no + 1)) AS nif FROM test_emp ORDER BY nif NULLS FIRST LIMIT 5; + greatestField SELECT GREATEST(emp_no - 1 + 3, ABS(emp_no) + 1) AS "greatest" FROM test_emp ORDER BY emp_no LIMIT 5; @@ -32,6 +38,9 @@ SELECT emp_no FROM test_emp WHERE GREATEST(10005, ABS(emp_no) + 1, null, emp_no greatestHaving SELECT GREATEST(10096, ABS(MAX(emp_no)) + 1) AS gt FROM test_emp GROUP BY languages HAVING gt >= 10098 ORDER BY languages; +greatestOrderBy +SELECT GREATEST(10096, ABS(emp_no + 1)) AS gt FROM test_emp ORDER BY gt LIMIT 10; + leastField SELECT LEAST(emp_no - 1 + 3, ABS(emp_no) + 1) AS "least" FROM test_emp ORDER BY emp_no LIMIT 5; @@ -40,3 +49,6 @@ SELECT emp_no FROM test_emp WHERE LEAST(10005, ABS(emp_no) + 1, null, emp_no - 1 leastHaving SELECT LEAST(10098, ABS(MAX(emp_no)) + 1) AS lt FROM test_emp GROUP BY languages HAVING lt >= 10095 ORDER BY languages; + +leastOrderBy +SELECT LEAST(10096, ABS(emp_no + 1)) AS lt FROM test_emp ORDER BY lt LIMIT 10; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ArbitraryConditionalFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ArbitraryConditionalFunction.java index 6d38037ec26..9a3c24c3729 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ArbitraryConditionalFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ArbitraryConditionalFunction.java @@ -61,6 +61,6 @@ public abstract class ArbitraryConditionalFunction extends ConditionalFunction { params.script(scriptTemplate.params()); } - return new ScriptTemplate(template.toString(), params.build(), dataType()); + return new ScriptTemplate(formatTemplate(template.toString()), params.build(), dataType()); } } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIf.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIf.java index 3d5b78182e6..0f8bb3f2085 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIf.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/NullIf.java @@ -75,7 +75,7 @@ public class NullIf extends ConditionalFunction { params.script(left.params()); params.script(right.params()); - return new ScriptTemplate(template, params.build(), dataType); + return new ScriptTemplate(formatTemplate(template), params.build(), dataType); } @Override 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 index 3d0e8ed0fab..b5b19004eee 100644 --- 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 @@ -46,10 +46,10 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS # # Null # - Object coalesce(java.util.List) - Object greatest(java.util.List) - Object least(java.util.List) - Object nullif(Object, Object) + def coalesce(java.util.List) + def greatest(java.util.List) + def least(java.util.List) + def nullif(Object, Object) # # Regex diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java index d4ee7bce36f..9085e8841f3 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java @@ -15,12 +15,15 @@ import org.elasticsearch.xpack.sql.analysis.index.MappingException; import org.elasticsearch.xpack.sql.expression.Expression; import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry; import org.elasticsearch.xpack.sql.expression.function.scalar.math.MathProcessor.MathOperation; +import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate; import org.elasticsearch.xpack.sql.parser.SqlParser; +import org.elasticsearch.xpack.sql.plan.logical.Aggregate; import org.elasticsearch.xpack.sql.plan.logical.Filter; import org.elasticsearch.xpack.sql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.sql.plan.logical.Project; import org.elasticsearch.xpack.sql.planner.QueryTranslator.QueryTranslation; import org.elasticsearch.xpack.sql.querydsl.agg.AggFilter; +import org.elasticsearch.xpack.sql.querydsl.agg.GroupByScriptKey; import org.elasticsearch.xpack.sql.querydsl.query.ExistsQuery; import org.elasticsearch.xpack.sql.querydsl.query.NotQuery; import org.elasticsearch.xpack.sql.querydsl.query.Query; @@ -392,4 +395,30 @@ public class QueryTranslatorTests extends ESTestCase { assertThat(aggFilter.scriptTemplate().params().toString(), startsWith("[{a=MAX(int){a->")); assertThat(aggFilter.scriptTemplate().params().toString(), endsWith(", {v=10}]")); } + + public void testTranslateCoalesce_GroupBy_Painless() { + LogicalPlan p = plan("SELECT COALESCE(int, 10) FROM test GROUP BY 1"); + assertTrue(p instanceof Aggregate); + Expression condition = ((Aggregate) p).groupings().get(0); + assertFalse(condition.foldable()); + QueryTranslator.GroupingContext groupingContext = QueryTranslator.groupBy(((Aggregate) p).groupings()); + assertNotNull(groupingContext); + ScriptTemplate scriptTemplate = ((GroupByScriptKey) groupingContext.tail).script(); + assertEquals("InternalSqlScriptUtils.coalesce([InternalSqlScriptUtils.docValue(doc,params.v0),params.v1])", + scriptTemplate.toString()); + assertEquals("[{v=int}, {v=10}]", scriptTemplate.params().toString()); + } + + public void testTranslateNullIf_GroupBy_Painless() { + LogicalPlan p = plan("SELECT NULLIF(int, 10) FROM test GROUP BY 1"); + assertTrue(p instanceof Aggregate); + Expression condition = ((Aggregate) p).groupings().get(0); + assertFalse(condition.foldable()); + QueryTranslator.GroupingContext groupingContext = QueryTranslator.groupBy(((Aggregate) p).groupings()); + assertNotNull(groupingContext); + ScriptTemplate scriptTemplate = ((GroupByScriptKey) groupingContext.tail).script(); + assertEquals("InternalSqlScriptUtils.nullif(InternalSqlScriptUtils.docValue(doc,params.v0),params.v1)", + scriptTemplate.toString()); + assertEquals("[{v=int}, {v=10}]", scriptTemplate.params().toString()); + } }