From 7a34ba35f7e362a541f52d8dc884118c193db7c9 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 19 Apr 2019 19:03:28 +0300 Subject: [PATCH] SQL: Fix bug with optimization of null related conditionals (#41355) The SimplifyConditional rule is removing NULL literals from those functions to simplify their evaluation. This happens in the Optimizer and a new instance of the conditional function is generated. Previously, the dataType was not set properly (defaulted to DataType.NULL) for those new instances and since the resolveType() wasn't called again it resulted in returning always null. E.g.: SELECT COALESCE(null, 'foo', null, 'bar') COALESCE(null, 'foo', null, 'bar') ----------------- null This issue was not visible before because the tests always used an alias for the conditional function which caused the resolveType() to be called which sets the dataType properly. E.g.: SELECT COALESCE(null, 'foo', null, 'bar') as c c ----------------- foo (cherry picked from commit c39980a65dd593363f1d8d1b038b26cb0ce02aaf) --- x-pack/plugin/sql/qa/src/main/resources/null.csv-spec | 7 +++++++ .../predicate/conditional/ConditionalFunction.java | 9 +++++++-- .../xpack/sql/optimizer/OptimizerTests.java | 6 ++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/null.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/null.csv-spec index 19541cf5d9f..610217b2333 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/null.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/null.csv-spec @@ -61,6 +61,13 @@ c:i ; coalesceMixed +SELECT COALESCE(null, 123, null, 321); + +COALESCE(null, 123, null, 321):i +123 +; + +coalesceMixedWithAlias SELECT COALESCE(null, 123, null, 321) AS c; c:i diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ConditionalFunction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ConditionalFunction.java index 3de85185e8a..b3841f09e82 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ConditionalFunction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ConditionalFunction.java @@ -25,7 +25,7 @@ import static org.elasticsearch.xpack.sql.util.StringUtils.ordinal; */ public abstract class ConditionalFunction extends ScalarFunction { - protected DataType dataType = DataType.NULL; + protected DataType dataType = null; ConditionalFunction(Source source, List fields) { super(source, fields); @@ -33,6 +33,12 @@ public abstract class ConditionalFunction extends ScalarFunction { @Override public DataType dataType() { + if (dataType == null) { + dataType = DataType.NULL; + for (Expression exp : children()) { + dataType = DataTypeConversion.commonType(dataType, exp.dataType()); + } + } return dataType; } @@ -61,7 +67,6 @@ public abstract class ConditionalFunction extends ScalarFunction { child.dataType().typeName)); } } - dataType = DataTypeConversion.commonType(dataType, child.dataType()); } return TypeResolution.TYPE_RESOLVED; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index c95206c29e9..eb8ac2b4d15 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -501,6 +501,7 @@ public class OptimizerTests extends ESTestCase { randomListOfNulls()))); assertEquals(1, e.children().size()); assertEquals(TRUE, e.children().get(0)); + assertEquals(DataType.BOOLEAN, e.dataType()); } private List randomListOfNulls() { @@ -514,6 +515,7 @@ public class OptimizerTests extends ESTestCase { assertEquals(Coalesce.class, e.getClass()); assertEquals(1, e.children().size()); assertEquals(TRUE, e.children().get(0)); + assertEquals(DataType.BOOLEAN, e.dataType()); } public void testSimplifyIfNullNulls() { @@ -527,11 +529,13 @@ public class OptimizerTests extends ESTestCase { assertEquals(IfNull.class, e.getClass()); assertEquals(1, e.children().size()); assertEquals(ONE, e.children().get(0)); + assertEquals(DataType.INTEGER, e.dataType()); e = new SimplifyConditional().rule(new IfNull(EMPTY, ONE, NULL)); assertEquals(IfNull.class, e.getClass()); assertEquals(1, e.children().size()); assertEquals(ONE, e.children().get(0)); + assertEquals(DataType.INTEGER, e.dataType()); } public void testFoldNullNotAppliedOnNullIf() { @@ -559,6 +563,7 @@ public class OptimizerTests extends ESTestCase { assertEquals(2, e.children().size()); assertEquals(ONE, e.children().get(0)); assertEquals(TWO, e.children().get(1)); + assertEquals(DataType.INTEGER, e.dataType()); } public void testSimplifyLeastNulls() { @@ -580,6 +585,7 @@ public class OptimizerTests extends ESTestCase { assertEquals(2, e.children().size()); assertEquals(ONE, e.children().get(0)); assertEquals(TWO, e.children().get(1)); + assertEquals(DataType.INTEGER, e.dataType()); } public void testConcatFoldingIsNotNull() {