ESQL: Do not discard disjunction conditions when is null/is not null might invalidate them by astefan · Pull Request #145941 · elastic/elasticsearch

astefan · 2026-04-08T20:33:49Z

This is closing (not fixing, because there is no reproduceable use case, nor there is a possible java execution path for that) #141579.
AI-assisted PR.

invalidate them

…141579_fix

elasticsearchmachine · 2026-04-09T12:30:15Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2026-04-09T12:30:16Z

Hi @astefan, I've created a changelog YAML for you.

astefan · 2026-04-09T13:05:24Z

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

+required_capability: fix_propagate_nullable_or_disjunction
+
+FROM employees
+| WHERE (gender IS NOT NULL OR emp_no > 10000) AND gender IS NULL


One of the tests that is the actual bug this PR is fixing.
Before this PR, this query returned 0 because emp_no > 10000 was lost.

bpintea

LG.
Not sure if worth it, but if going through another CI anyways, I'd maybe only a test with a composite expressions, not just fields. Seeing that semantic equality (a + b IS NOT NULL ... OR b + a > 0).

astefan · 2026-04-09T14:37:02Z

LG. Not sure if worth it, but if going through another CI anyways, I'd maybe only a test with a composite expressions, not just fields. Seeing that semantic equality (a + b IS NOT NULL ... OR b + a > 0).

Yep, doing exactly that as I write this. Thank you for reviewing.

…141579_fix

…into 141579_fix

alex-spies

Thanks @astefan , the productive changes look right to me. Mostly have a question :)

alex-spies · 2026-04-09T17:02:11Z

...ql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateNullable.java

+     * Substitutes {@code IS NULL} and {@code IS NOT NULL} predicates — and, when the field is known to be {@code NULL},
+     * direct field references — throughout {@code exp}.
+     * <p>
+     * When {@code isNullResult} is {@code true} (field constrained to {@code IS NULL}):
+     * direct field occurrences become {@code null}, {@code IS NULL(field)} becomes {@code true},
+     * {@code IS NOT NULL(field)} becomes {@code false}.
+     * <p>
+     * When {@code isNullResult} is {@code false} (field constrained to {@code IS NOT NULL}):
+     * only the predicate forms are substituted ({@code IS NULL} → {@code false},
+     * {@code IS NOT NULL} → {@code true}); the field value itself is left intact because its
+     * concrete value is unknown.
+     * <p>
+     * This preserves surviving OR branches instead of discarding them. For example:
+     * {@code (a IS NOT NULL OR p) AND a IS NULL} → {@code OR(false, p) AND a IS NULL},
+     * which {@code BooleanSimplification} then reduces to {@code p AND a IS NULL}.


nit: This comment is longer than the method impl and doesn't say much except for the last paragraph with the example.

Tbh, usually I don't like what method comments AI adds, but in this particular case I went ahead with these because:

this edge case that this PR covers is tricky enough to understand to warrant a big fat method comment

optimization rules are notoriously hard to understand for someone who didn't either developed that thing or didn't spend quite some time going over edge cases to understand it

With all its ugliness, I will keep this method comment for the reasons above.

alex-spies · 2026-04-09T17:10:58Z

...ql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateNullable.java

        if (exp instanceof Coalesce) {
+            // Remove direct COALESCE arguments that are exactly the null field; the remaining
+            // arguments may still reference the field and are handled by substituteNullability below.
            List<Expression> newChildren = new ArrayList<>(exp.children());
            newChildren.removeIf(e -> e.semanticEquals(nullExp));
            if (newChildren.size() != exp.children().size() && newChildren.size() > 0) { // coalesce needs at least one input
-                return exp.replaceChildren(newChildren);
+                base = exp.replaceChildren(newChildren);
            }
        }


nit: It's a bit weird that we only look at top-level COALESCEs, but we descend fully into the expression to look for more IS NULL/IS NOT NULLs. Maybe this can be simplified?

Maybe there are things to simplify further, no argument against that. I kept this code as is because:

this PR is really targeted and surgical, if there is room for more, that "more" deserves a new PR and specific edge cases to cover. Those edge cases must be called out somehow (new issue, bug we discover, bug an user discovers, CI failure from generative testing etc).

this code existed before and I didn't want to touch it, because it's not this PR's concern tbh.

I believe I dug deep enough with the many tests I added to be happy with the PR as is now :-).

alex-spies · 2026-04-09T17:12:14Z

...ql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateNullable.java

-        return Literal.of(exp, null);
+        return substituteNullability(base, nullExp, true);


Oh, thank you. It's a bit hard to spot with the convoluted replacer logic, but we used to return literal nulls for the whole expression, rather than descending into it and replacing occurences of the null field. That was very wrong.

alex-spies · 2026-04-09T17:15:50Z

...ql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateNullable.java

+     * {@code (a IS NOT NULL OR p) AND a IS NULL} → {@code OR(false, p) AND a IS NULL},
+     * which {@code BooleanSimplification} then reduces to {@code p AND a IS NULL}.
+     */
+    private static Expression substituteNullability(Expression exp, Expression field, boolean isNullResult) {


Question: why do we have to special case IsNull and IsNotNull expressions? Is it because our constant folding only folds fully foldable expressions, but doesn't attempt to partially fold subexpressions? Maybe a more complete solution would be to make ConstantFolding attempt partial folding?

ConstantFolding, FoldNull and PropagateNullable all share some conceptual stuff: dealing with a sort of constant. When that constant can be null/is null is a variant of constant folding but is special enough (see is null/is not null and the concept of Nullability) that it needs one-two separate rules. It is tricky (and I'd be afraid to attempt this) to change these core optimization rules just to try doing a generic partial folding without any good/strong reason. For example, if aggregation by constants would be attempted by someone as a generic fix, that would be a good time to consider this here specific partial folding on ConstantFolding.

astefan added 4 commits April 8, 2026 23:32

Do not discard disjunction conditions when is null/is not null might

ed0ebb2

invalidate them

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

49c3664

…141579_fix

Fix nonNullify as well

a905d6e

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

9400e9a

…141579_fix

astefan requested review from alex-spies and bpintea April 9, 2026 12:29

astefan added >bug :Analytics/ES|QL AKA ESQL v9.4.0 v9.3.4 auto-backport Automatically create backport pull requests when merged labels Apr 9, 2026

astefan marked this pull request as ready for review April 9, 2026 12:29

Merge branch 'main' into 141579_fix

84488a7

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 9, 2026

Update docs/changelog/145941.yaml

0cc0edc

astefan commented Apr 9, 2026

View reviewed changes

bpintea approved these changes Apr 9, 2026

View reviewed changes

astefan and others added 6 commits April 9, 2026 17:41

Add more tests and clean up a bit the unit tests

d51446b

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

bc46892

…141579_fix

Merge branch '141579_fix' of https://github.com/astefan/elasticsearch …

c07ab24

…into 141579_fix

Even more tests

bab7921

Merge branch 'main' into 141579_fix

3e10dfc

Merge branch 'main' into 141579_fix

b3afc7c

alex-spies approved these changes Apr 9, 2026

View reviewed changes

astefan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 9, 2026

Merge branch 'main' into 141579_fix

4026811

		return Literal.of(exp, null);
		return substituteNullability(base, nullExp, true);

Conversation

astefan commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Apr 9, 2026

Uh oh!

elasticsearchmachine commented Apr 9, 2026

Uh oh!

astefan Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

astefan commented Apr 9, 2026

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

alex-spies Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

alex-spies Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

astefan Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

astefan commented Apr 8, 2026 •

edited

Loading

astefan Apr 9, 2026 •

edited

Loading