Skip to content

Commit 1aba9f3

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 001bbdb commit 1aba9f3

File tree

4 files changed

+23
-1
lines changed

4 files changed

+23
-1
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/ArbitraryConditionalFunction.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.elasticsearch.xpack.sql.expression.gen.script.ScriptTemplate;
1414
import org.elasticsearch.xpack.sql.expression.predicate.conditional.ConditionalProcessor.ConditionalOperation;
1515
import org.elasticsearch.xpack.sql.tree.Source;
16+
import org.elasticsearch.xpack.sql.type.DataType;
1617
import org.elasticsearch.xpack.sql.type.DataTypeConversion;
1718

1819
import java.util.ArrayList;
@@ -35,6 +36,7 @@ public abstract class ArbitraryConditionalFunction extends ConditionalFunction {
3536

3637
@Override
3738
protected TypeResolution resolveType() {
39+
dataType = DataType.NULL;
3840
for (Expression e : children()) {
3941
dataType = DataTypeConversion.commonType(dataType, e.dataType());
4042
}

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
1313
import org.elasticsearch.xpack.sql.tree.Source;
1414
import org.elasticsearch.xpack.sql.type.DataType;
15+
import org.elasticsearch.xpack.sql.type.DataTypeConversion;
1516

1617
import java.util.List;
1718

@@ -20,14 +21,20 @@
2021
*/
2122
public abstract class ConditionalFunction extends ScalarFunction {
2223

23-
protected DataType dataType = DataType.NULL;
24+
protected DataType dataType = null;
2425

2526
ConditionalFunction(Source source, List<Expression> fields) {
2627
super(source, fields);
2728
}
2829

2930
@Override
3031
public DataType dataType() {
32+
if (dataType == null) {
33+
dataType = DataType.NULL;
34+
for (Expression exp : children()) {
35+
dataType = DataTypeConversion.commonType(dataType, exp.dataType());
36+
}
37+
}
3138
return dataType;
3239
}
3340

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
@@ -481,6 +481,7 @@ public void testSimplifyCoalesceRandomNullsWithValue() {
481481
randomListOfNulls())));
482482
assertEquals(1, e.children().size());
483483
assertEquals(Literal.TRUE, e.children().get(0));
484+
assertEquals(DataType.BOOLEAN, e.dataType());
484485
}
485486

486487
private List<Expression> randomListOfNulls() {
@@ -494,6 +495,7 @@ public void testSimplifyCoalesceFirstLiteral() {
494495
assertEquals(Coalesce.class, e.getClass());
495496
assertEquals(1, e.children().size());
496497
assertEquals(Literal.TRUE, e.children().get(0));
498+
assertEquals(DataType.BOOLEAN, e.dataType());
497499
}
498500

499501
public void testSimplifyIfNullNulls() {
@@ -507,11 +509,13 @@ public void testSimplifyIfNullWithNullAndValue() {
507509
assertEquals(IfNull.class, e.getClass());
508510
assertEquals(1, e.children().size());
509511
assertEquals(ONE, e.children().get(0));
512+
assertEquals(DataType.INTEGER, e.dataType());
510513

511514
e = new SimplifyConditional().rule(new IfNull(EMPTY, ONE, Literal.NULL));
512515
assertEquals(IfNull.class, e.getClass());
513516
assertEquals(1, e.children().size());
514517
assertEquals(ONE, e.children().get(0));
518+
assertEquals(DataType.INTEGER, e.dataType());
515519
}
516520

517521
public void testFoldNullNotAppliedOnNullIf() {
@@ -539,6 +543,7 @@ public void testSimplifyGreatestRandomNullsWithValue() {
539543
assertEquals(2, e.children().size());
540544
assertEquals(ONE, e.children().get(0));
541545
assertEquals(TWO, e.children().get(1));
546+
assertEquals(DataType.INTEGER, e.dataType());
542547
}
543548

544549
public void testSimplifyLeastNulls() {
@@ -560,6 +565,7 @@ public void testSimplifyLeastRandomNullsWithValue() {
560565
assertEquals(2, e.children().size());
561566
assertEquals(ONE, e.children().get(0));
562567
assertEquals(TWO, e.children().get(1));
568+
assertEquals(DataType.INTEGER, e.dataType());
563569
}
564570

565571
public void testConcatFoldingIsNotNull() {

0 commit comments

Comments
 (0)