Skip to content

Commit 2cccf13

Browse files
astefanastefan
authored
ES|QL: Skip CASE function from InferIsNotNull rule checks (#113123)
* Skip CASE function from InferIsNotNull rule checks
1 parent 7c65ad5 commit 2cccf13

File tree

5 files changed

+108
-41
lines changed

5 files changed

+108
-41
lines changed

‎docs/changelog/113123.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 113123
2+
summary: "ES|QL: Skip CASE function from `InferIsNotNull` rule checks"
3+
area: ES|QL
4+
type: bug
5+
issues:
6+
- 112704

‎x-pack/plugin/esql/qa/testFixtures/src/main/resources/conditional.csv-spec‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,3 +261,23 @@ warning:Line 1:38: java.lang.IllegalArgumentException: single-value function enc
261261

262262
a:integer | b:integer | same:boolean
263263
;
264+
265+
caseOnTheValue_NotOnTheField
266+
required_capability: fixed_wrong_is_not_null_check_on_case
267+
268+
FROM employees
269+
| WHERE emp_no < 10022 AND emp_no > 10012
270+
| KEEP languages, emp_no
271+
| EVAL eval = CASE(languages == 1, null, languages == 2, "bilingual", languages > 2, "multilingual", languages IS NULL, "languages is null")
272+
| SORT languages, emp_no
273+
| WHERE eval IS NOT NULL;
274+
275+
languages:integer| emp_no:integer|eval:keyword
276+
2 |10016 |bilingual
277+
2 |10017 |bilingual
278+
2 |10018 |bilingual
279+
5 |10014 |multilingual
280+
5 |10015 |multilingual
281+
null |10020 |languages is null
282+
null |10021 |languages is null
283+
;

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,13 @@ public enum Cap {
314314
/**
315315
* QSTR function
316316
*/
317-
QSTR_FUNCTION(true);
317+
QSTR_FUNCTION(true),
318+
319+
/**
320+
* Don't optimize CASE IS NOT NULL function by not requiring the fields to be not null as well.
321+
* https://github.com/elastic/elasticsearch/issues/112704
322+
*/
323+
FIXED_WRONG_IS_NOT_NULL_CHECK_ON_CASE;
318324

319325
private final boolean snapshotOnly;
320326
private final FeatureFlag featureFlag;

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/InferIsNotNull.java‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.elasticsearch.xpack.esql.core.expression.predicate.nulls.IsNotNull;
1616
import org.elasticsearch.xpack.esql.core.rule.Rule;
1717
import org.elasticsearch.xpack.esql.core.util.CollectionUtils;
18+
import org.elasticsearch.xpack.esql.expression.function.scalar.conditional.Case;
1819
import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce;
1920
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
2021

@@ -24,8 +25,7 @@
2425
import static java.util.Collections.emptySet;
2526

2627
/**
27-
* Simplify IsNotNull targets by resolving the underlying expression to its root fields with unknown
28-
* nullability.
28+
* Simplify IsNotNull targets by resolving the underlying expression to its root fields.
2929
* e.g.
3030
* (x + 1) / 2 IS NOT NULL --> x IS NOT NULL AND (x+1) / 2 IS NOT NULL
3131
* SUBSTRING(x, 3) > 4 IS NOT NULL --> x IS NOT NULL AND SUBSTRING(x, 3) > 4 IS NOT NULL
@@ -85,7 +85,7 @@ protected Set<Expression> resolveExpressionAsRootAttributes(Expression exp, Attr
8585

8686
private boolean doResolve(Expression exp, AttributeMap<Expression> aliases, Set<Expression> resolvedExpressions) {
8787
boolean changed = false;
88-
// check if the expression can be skipped or is not nullabe
88+
// check if the expression can be skipped
8989
if (skipExpression(exp)) {
9090
resolvedExpressions.add(exp);
9191
} else {
@@ -106,6 +106,13 @@ private boolean doResolve(Expression exp, AttributeMap<Expression> aliases, Set<
106106
}
107107

108108
private static boolean skipExpression(Expression e) {
109-
return e instanceof Coalesce;
109+
// These two functions can have a complex set of expressions as arguments that can mess up the simplification we are trying to add.
110+
// If there is a "case(f is null, null, ...) is not null" expression,
111+
// assuming that "case(f is null.....) is not null AND f is not null" (what this rule is doing) is a wrong assumption because
112+
// the "case" function will want both null "f" and not null "f". Doing it like this contradicts the condition inside case, so we
113+
// must avoid these cases.
114+
// We could be smarter and look inside "case" and "coalesce" to see if there is any comparison of fields with "null" but,
115+
// the complexity is too high to warrant an attempt _now_.
116+
return e instanceof Coalesce || e instanceof Case;
110117
}
111118
}

‎x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java‎

Lines changed: 64 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.elasticsearch.xpack.esql.core.type.DataType;
3030
import org.elasticsearch.xpack.esql.core.type.EsField;
3131
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
32+
import org.elasticsearch.xpack.esql.expression.function.scalar.conditional.Case;
3233
import org.elasticsearch.xpack.esql.expression.function.scalar.nulls.Coalesce;
3334
import org.elasticsearch.xpack.esql.expression.function.scalar.string.StartsWith;
3435
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Add;
@@ -58,8 +59,11 @@
5859

5960
import static java.util.Collections.emptyMap;
6061
import static org.elasticsearch.xpack.esql.EsqlTestUtils.L;
62+
import static org.elasticsearch.xpack.esql.EsqlTestUtils.ONE;
6163
import static org.elasticsearch.xpack.esql.EsqlTestUtils.TEST_SEARCH_STATS;
6264
import static org.elasticsearch.xpack.esql.EsqlTestUtils.TEST_VERIFIER;
65+
import static org.elasticsearch.xpack.esql.EsqlTestUtils.THREE;
66+
import static org.elasticsearch.xpack.esql.EsqlTestUtils.TWO;
6367
import static org.elasticsearch.xpack.esql.EsqlTestUtils.as;
6468
import static org.elasticsearch.xpack.esql.EsqlTestUtils.getFieldAttribute;
6569
import static org.elasticsearch.xpack.esql.EsqlTestUtils.greaterThanOf;
@@ -81,10 +85,6 @@ public class LocalLogicalPlanOptimizerTests extends ESTestCase {
8185
private static LogicalPlanOptimizer logicalOptimizer;
8286
private static Map<String, EsField> mapping;
8387

84-
private static final Literal ONE = L(1);
85-
private static final Literal TWO = L(2);
86-
private static final Literal THREE = L(3);
87-
8888
@BeforeClass
8989
public static void init() {
9090
parser = new EsqlParser();
@@ -386,38 +386,6 @@ public void testMissingFieldInFilterNoProjection() {
386386
);
387387
}
388388

389-
public void testIsNotNullOnCoalesce() {
390-
var plan = localPlan("""
391-
from test
392-
| where coalesce(emp_no, salary) is not null
393-
""");
394-
395-
var limit = as(plan, Limit.class);
396-
var filter = as(limit.child(), Filter.class);
397-
var inn = as(filter.condition(), IsNotNull.class);
398-
var coalesce = as(inn.children().get(0), Coalesce.class);
399-
assertThat(Expressions.names(coalesce.children()), contains("emp_no", "salary"));
400-
var source = as(filter.child(), EsRelation.class);
401-
}
402-
403-
public void testIsNotNullOnExpression() {
404-
var plan = localPlan("""
405-
from test
406-
| eval x = emp_no + 1
407-
| where x is not null
408-
""");
409-
410-
var limit = as(plan, Limit.class);
411-
var filter = as(limit.child(), Filter.class);
412-
var inn = as(filter.condition(), IsNotNull.class);
413-
assertThat(Expressions.names(inn.children()), contains("x"));
414-
var eval = as(filter.child(), Eval.class);
415-
filter = as(eval.child(), Filter.class);
416-
inn = as(filter.condition(), IsNotNull.class);
417-
assertThat(Expressions.names(inn.children()), contains("emp_no"));
418-
var source = as(filter.child(), EsRelation.class);
419-
}
420-
421389
public void testSparseDocument() throws Exception {
422390
var query = """
423391
from large
@@ -516,6 +484,66 @@ public void testIsNotNullOnFunctionWithTwoFields() {
516484
assertEquals(expected, new InferIsNotNull().apply(f));
517485
}
518486

487+
public void testIsNotNullOnCoalesce() {
488+
var plan = localPlan("""
489+
from test
490+
| where coalesce(emp_no, salary) is not null
491+
""");
492+
493+
var limit = as(plan, Limit.class);
494+
var filter = as(limit.child(), Filter.class);
495+
var inn = as(filter.condition(), IsNotNull.class);
496+
var coalesce = as(inn.children().get(0), Coalesce.class);
497+
assertThat(Expressions.names(coalesce.children()), contains("emp_no", "salary"));
498+
var source = as(filter.child(), EsRelation.class);
499+
}
500+
501+
public void testIsNotNullOnExpression() {
502+
var plan = localPlan("""
503+
from test
504+
| eval x = emp_no + 1
505+
| where x is not null
506+
""");
507+
508+
var limit = as(plan, Limit.class);
509+
var filter = as(limit.child(), Filter.class);
510+
var inn = as(filter.condition(), IsNotNull.class);
511+
assertThat(Expressions.names(inn.children()), contains("x"));
512+
var eval = as(filter.child(), Eval.class);
513+
filter = as(eval.child(), Filter.class);
514+
inn = as(filter.condition(), IsNotNull.class);
515+
assertThat(Expressions.names(inn.children()), contains("emp_no"));
516+
var source = as(filter.child(), EsRelation.class);
517+
}
518+
519+
public void testIsNotNullOnCase() {
520+
var plan = localPlan("""
521+
from test
522+
| where case(emp_no > 10000, "1", salary < 50000, "2", first_name) is not null
523+
""");
524+
525+
var limit = as(plan, Limit.class);
526+
var filter = as(limit.child(), Filter.class);
527+
var inn = as(filter.condition(), IsNotNull.class);
528+
var caseF = as(inn.children().get(0), Case.class);
529+
assertThat(Expressions.names(caseF.children()), contains("emp_no > 10000", "\"1\"", "salary < 50000", "\"2\"", "first_name"));
530+
var source = as(filter.child(), EsRelation.class);
531+
}
532+
533+
public void testIsNotNullOnCase_With_IS_NULL() {
534+
var plan = localPlan("""
535+
from test
536+
| where case(emp_no IS NULL, "1", salary IS NOT NULL, "2", first_name) is not null
537+
""");
538+
539+
var limit = as(plan, Limit.class);
540+
var filter = as(limit.child(), Filter.class);
541+
var inn = as(filter.condition(), IsNotNull.class);
542+
var caseF = as(inn.children().get(0), Case.class);
543+
assertThat(Expressions.names(caseF.children()), contains("emp_no IS NULL", "\"1\"", "salary IS NOT NULL", "\"2\"", "first_name"));
544+
var source = as(filter.child(), EsRelation.class);
545+
}
546+
519547
private IsNotNull isNotNull(Expression field) {
520548
return new IsNotNull(EMPTY, field);
521549
}

0 commit comments

Comments
 (0)