From ed09efd77330065efa15ea0c2786610f78558810 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 18 Apr 2019 18:41:49 +0300 Subject: [PATCH 1/3] SQL: Fix bug with optimization of null related conditionals 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`. This issue was not visible before because the tests always used an alias for the conditional function which caused the `resolvedType()` to be called which sets the dataType properly. --- x-pack/plugin/sql/qa/src/main/resources/null.csv-spec | 7 +++++++ .../predicate/conditional/ConditionalFunction.java | 8 +++++++- .../elasticsearch/xpack/sql/optimizer/OptimizerTests.java | 6 ++++++ 3 files changed, 20 insertions(+), 1 deletion(-) 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 19541cf5d9f32..610217b233314 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 3de85185e8a4f..f8347c714ee21 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 @@ -29,6 +29,7 @@ public abstract class ConditionalFunction extends ScalarFunction { ConditionalFunction(Source source, List fields) { super(source, fields); + setDataType(); } @Override @@ -61,7 +62,6 @@ protected TypeResolution resolveType() { child.dataType().typeName)); } } - dataType = DataTypeConversion.commonType(dataType, child.dataType()); } return TypeResolution.TYPE_RESOLVED; } @@ -70,4 +70,10 @@ protected TypeResolution resolveType() { public Nullability nullable() { return Nullability.UNKNOWN; } + + private void setDataType() { + for (Expression exp : children()) { + dataType = DataTypeConversion.commonType(dataType, exp.dataType()); + } + } } 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 a23d88b595630..2506e3df70220 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 @@ -498,6 +498,7 @@ public void testSimplifyCoalesceRandomNullsWithValue() { randomListOfNulls()))); assertEquals(1, e.children().size()); assertEquals(TRUE, e.children().get(0)); + assertEquals(DataType.BOOLEAN, e.dataType()); } private List randomListOfNulls() { @@ -511,6 +512,7 @@ public void testSimplifyCoalesceFirstLiteral() { assertEquals(Coalesce.class, e.getClass()); assertEquals(1, e.children().size()); assertEquals(TRUE, e.children().get(0)); + assertEquals(DataType.BOOLEAN, e.dataType()); } public void testSimplifyIfNullNulls() { @@ -524,11 +526,13 @@ public void testSimplifyIfNullWithNullAndValue() { 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() { @@ -556,6 +560,7 @@ public void testSimplifyGreatestRandomNullsWithValue() { 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() { @@ -577,6 +582,7 @@ public void testSimplifyLeastRandomNullsWithValue() { 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() { From 5fddec587fc2126c75f82f3e772f536fbce15dd3 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Thu, 18 Apr 2019 23:42:38 +0300 Subject: [PATCH 2/3] Use lazy data type resolution --- .../predicate/conditional/ConditionalFunction.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 f8347c714ee21..cf0bcb7c98151 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 @@ -26,14 +26,20 @@ public abstract class ConditionalFunction extends ScalarFunction { protected DataType dataType = DataType.NULL; + private boolean lazyDataTypeResolution; ConditionalFunction(Source source, List fields) { super(source, fields); - setDataType(); } @Override public DataType dataType() { + if (lazyDataTypeResolution == false) { + for (Expression exp : children()) { + dataType = DataTypeConversion.commonType(dataType, exp.dataType()); + } + lazyDataTypeResolution = true; + } return dataType; } @@ -70,10 +76,4 @@ protected TypeResolution resolveType() { public Nullability nullable() { return Nullability.UNKNOWN; } - - private void setDataType() { - for (Expression exp : children()) { - dataType = DataTypeConversion.commonType(dataType, exp.dataType()); - } - } } From e81372ffd9afa78fe67f5040e21714207d77262b Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 19 Apr 2019 11:00:30 +0300 Subject: [PATCH 3/3] remove lazy init boolean --- .../predicate/conditional/ConditionalFunction.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 cf0bcb7c98151..b3841f09e825e 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,8 +25,7 @@ */ public abstract class ConditionalFunction extends ScalarFunction { - protected DataType dataType = DataType.NULL; - private boolean lazyDataTypeResolution; + protected DataType dataType = null; ConditionalFunction(Source source, List fields) { super(source, fields); @@ -34,11 +33,11 @@ public abstract class ConditionalFunction extends ScalarFunction { @Override public DataType dataType() { - if (lazyDataTypeResolution == false) { + if (dataType == null) { + dataType = DataType.NULL; for (Expression exp : children()) { dataType = DataTypeConversion.commonType(dataType, exp.dataType()); } - lazyDataTypeResolution = true; } return dataType; }