Skip to content

Commit 7a34ba3

Browse files
committed
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 c39980a)
1 parent d989079 commit 7a34ba3

File tree

3 files changed

+20
-2
lines changed

3 files changed

+20
-2
lines changed

x-pack/plugin/sql/qa/src/main/resources/null.csv-spec

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,13 @@ c:i
6161
;
6262

6363
coalesceMixed
64+
SELECT COALESCE(null, 123, null, 321);
65+
66+
COALESCE(null, 123, null, 321):i
67+
123
68+
;
69+
70+
coalesceMixedWithAlias
6471
SELECT COALESCE(null, 123, null, 321) AS c;
6572

6673
c:i

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/ConditionalFunction.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,20 @@
2525
*/
2626
public abstract class ConditionalFunction extends ScalarFunction {
2727

28-
protected DataType dataType = DataType.NULL;
28+
protected DataType dataType = null;
2929

3030
ConditionalFunction(Source source, List<Expression> fields) {
3131
super(source, fields);
3232
}
3333

3434
@Override
3535
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+
}
3642
return dataType;
3743
}
3844

@@ -61,7 +67,6 @@ protected TypeResolution resolveType() {
6167
child.dataType().typeName));
6268
}
6369
}
64-
dataType = DataTypeConversion.commonType(dataType, child.dataType());
6570
}
6671
return TypeResolution.TYPE_RESOLVED;
6772
}

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,7 @@ public void testSimplifyCoalesceRandomNullsWithValue() {
501501
randomListOfNulls())));
502502
assertEquals(1, e.children().size());
503503
assertEquals(TRUE, e.children().get(0));
504+
assertEquals(DataType.BOOLEAN, e.dataType());
504505
}
505506

506507
private List<Expression> randomListOfNulls() {
@@ -514,6 +515,7 @@ public void testSimplifyCoalesceFirstLiteral() {
514515
assertEquals(Coalesce.class, e.getClass());
515516
assertEquals(1, e.children().size());
516517
assertEquals(TRUE, e.children().get(0));
518+
assertEquals(DataType.BOOLEAN, e.dataType());
517519
}
518520

519521
public void testSimplifyIfNullNulls() {
@@ -527,11 +529,13 @@ public void testSimplifyIfNullWithNullAndValue() {
527529
assertEquals(IfNull.class, e.getClass());
528530
assertEquals(1, e.children().size());
529531
assertEquals(ONE, e.children().get(0));
532+
assertEquals(DataType.INTEGER, e.dataType());
530533

531534
e = new SimplifyConditional().rule(new IfNull(EMPTY, ONE, NULL));
532535
assertEquals(IfNull.class, e.getClass());
533536
assertEquals(1, e.children().size());
534537
assertEquals(ONE, e.children().get(0));
538+
assertEquals(DataType.INTEGER, e.dataType());
535539
}
536540

537541
public void testFoldNullNotAppliedOnNullIf() {
@@ -559,6 +563,7 @@ public void testSimplifyGreatestRandomNullsWithValue() {
559563
assertEquals(2, e.children().size());
560564
assertEquals(ONE, e.children().get(0));
561565
assertEquals(TWO, e.children().get(1));
566+
assertEquals(DataType.INTEGER, e.dataType());
562567
}
563568

564569
public void testSimplifyLeastNulls() {
@@ -580,6 +585,7 @@ public void testSimplifyLeastRandomNullsWithValue() {
580585
assertEquals(2, e.children().size());
581586
assertEquals(ONE, e.children().get(0));
582587
assertEquals(TWO, e.children().get(1));
588+
assertEquals(DataType.INTEGER, e.dataType());
583589
}
584590

585591
public void testConcatFoldingIsNotNull() {

0 commit comments

Comments
 (0)