SQL: Fix bug with optimization of null related conditionals (#41355)

matriv · web-flow · commit c39980a65dd5 · 2019-04-19T19:03:28.000+03:00
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
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
@@ -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
@@ -25,14 +25,20 @@
  */
 public abstract class ConditionalFunction extends ScalarFunction {
 
-    protected DataType dataType = DataType.NULL;
+    protected DataType dataType = null;
 
     ConditionalFunction(Source source, List<Expression> fields) {
         super(source, fields);
     }
 
     @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 @@ protected TypeResolution resolveType() {
                         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
@@ -501,6 +501,7 @@ public void testSimplifyCoalesceRandomNullsWithValue() {
                         randomListOfNulls())));
         assertEquals(1, e.children().size());
         assertEquals(TRUE, e.children().get(0));
+        assertEquals(DataType.BOOLEAN, e.dataType());
     }
 
     private List<Expression> randomListOfNulls() {
@@ -514,6 +515,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() {
@@ -527,11 +529,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() {
@@ -559,6 +563,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() {
@@ -580,6 +585,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() {

Original file line number	Diff line number	Diff line change
`@@ -25,14 +25,20 @@`
`25`	`25`	`*/`
`26`	`26`	`public abstract class ConditionalFunction extends ScalarFunction {`
`27`	`27`
`28`		`- protected DataType dataType = DataType.NULL;`
	`28`	`+ protected DataType dataType = null;`
`29`	`29`
`30`	`30`	`ConditionalFunction(Source source, List<Expression> fields) {`
`31`	`31`	`super(source, fields);`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`@Override`
`35`	`35`	`public DataType dataType() {`
	`36`	`+ if (dataType == null) {`
	`37`	`+ dataType = DataType.NULL;`
	`38`	`+ for (Expression exp : children()) {`
	`39`	`+ dataType = DataTypeConversion.commonType(dataType, exp.dataType());`
	`40`	`+ }`
	`41`	`+ }`
`36`	`42`	`return dataType;`
`37`	`43`	`}`
`38`	`44`
`@@ -61,7 +67,6 @@ protected TypeResolution resolveType() {`
`61`	`67`	`child.dataType().typeName));`
`62`	`68`	`}`
`63`	`69`	`}`
`64`		`- dataType = DataTypeConversion.commonType(dataType, child.dataType());`
`65`	`70`	`}`
`66`	`71`	`return TypeResolution.TYPE_RESOLVED;`
`67`	`72`	`}`