ESQL: Rework `isNull` #118101

nik9000 · 2024-12-05T18:10:35Z

This reworks Expressions#isNull so it only matches the null literal. This doesn't super change the way ESQL works because we already rewrite things that fold into null into the null literal. It's just that, now, isNull won't return true for things that fold to null - only things that have already folded to null.

This is important because fold can be quite expensive so we're better
off keeping the results of it when possible. Which is what the constant
folding rules do.

This reworks `Expressions#isNull` so it only matches the `null` literal. This doesn't super change the way ESQL works because we already rewrite things that `fold` into `null` into the `null` literal. It's just that, now, `isNull` won't return `true` for things that *fold* to null - only things that have *already* folded to null. This is important because `fold` can be quite expensive so we're better off keeping the results of it when possible. Which is what the constant folding rules *do*.

elasticsearchmachine · 2024-12-05T18:10:59Z

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

nik9000 · 2024-12-06T14:25:39Z

So my statement:

This doesn't super change the way ESQL works because we already rewrite things that fold into null into the null literal.

Is how I believe we work. But I think folks who own various bits of code should double check me.

nik9000 · 2024-12-06T14:30:26Z

...plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/FoldNull.java

        // P.S. this could be done inside the Aggregate but this place better centralizes the logic
        if (e instanceof AggregateFunction agg) {
-            if (Expressions.isNull(agg.filter())) {
+            if (Expressions.isGuaranteedNull(agg.filter())) {


I'm pretty sure the changes to this file are fine because, if we don't fold to null immediately, we will fold constants, which will yield literal null, which will then fold null here.

nik9000 · 2024-12-06T14:31:25Z

.../src/main/java/org/elasticsearch/xpack/esql/expression/predicate/operator/comparison/In.java

    @Override
    public Object fold() {
-        if (Expressions.isNull(value) || list.stream().allMatch(Expressions::isNull)) {
+        if (Expressions.isGuaranteedNull(value) || list.stream().allMatch(Expressions::isGuaranteedNull)) {


I'm curious about this one. I feel like it's tricky to be sure this is right.

I'm going to try removing this line from ESQL entirely to see what happens.... Learning!

Only one test fails if I zap this - testFoldNullListInToLocalRelation - which I don't really have the background to know. I don't think we should remove this, but I want to make sure this is still running properly in this way.

nik9000 · 2024-12-06T14:32:35Z

...in/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneFilters.java

                return filter.child();
            }
-            if (FALSE.equals(condition) || Expressions.isNull(condition)) {
+            if (FALSE.equals(condition) || Expressions.isGuaranteedNull(condition)) {


I'm pretty sure these are fine because we'll have rewritten all null valued expressions into literal nulls by the time we make it here. But I'd love is someone could confirm that we have a test for the plan bits.

And that someone could be me....

astefan · 2024-12-06T16:25:31Z

I am a bit surprised that the PR passes :-))).

I'll have a deeper look, though. There are many usecases that we do not sort-of support now that are related to aggregations - see #112392. That PR is a daring attempt at fixing the missing bits, but aggregations folding (as a concept) needs the happen otherwise that PR introduces way too many workarounds just to make some use cases work. I will close it and create a sort of meta issue for constant folding instead.

Leaving that PR aside, the change here with isGuaranteedNull expects the method to be used at the "right" time in the planning process where, as you say, things are already folded to a Literal. I need to take some example for a spin and try to break flow.

nik9000 · 2024-12-06T16:28:07Z

expects the method to be used at the "right" time

Yeah! I hate that. But I think the alternative is throwing away a bunch of work after folding. Sometimes allocating a lot of stuff.

costin

LGTM

astefan

I've given some more thought to this. Conceptually, isNull was doing too much I think by actually temporarily folding the value and checking it. Whereas the operation of the expression being folded (and actually being replaced as null in the entire tree) was happening elsewhere (Literal,of(e) via ConstantFolding or right here in FoldNull).

bpintea

Wondering what prompted the change. Repeatedly folding (and throwing away the results) is wasteful, but not that many rules get to run before isNull that do fold.

But at this scale, maybe placing ConstantFolding (and maybe PartiallyFoldCase) ahead of FoldNull in the optimiser would also make sense -- maybe push these three to be the first rules?

bpintea · 2024-12-06T17:20:43Z

...plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/expression/Expressions.java

+     * into a {@link Literal} containing {@code null} which will return
+     * {@code true} from here.
+     */
+    public static boolean isGuaranteedNull(Expression e) {


Nit: isGuaranteedNull makes me wonder what is "null" and more relevant when is it not guaranteed :)
I'd find something like isNullTypeOrLiteral clearer, but just a nitty nit.

Fair enough. I think I'm going to keep it because everything feels not so great. Maybe valueIsGuaranteedNull or something? I'm not sure that makes it more descriptive.

nik9000 · 2024-12-06T17:48:28Z

Seeing approvals, I'm going to merge on this so I can build on it. If anyone can come up with any fun extra test cases, let's make followup. And

But at this scale, maybe placing ConstantFolding (and maybe PartiallyFoldCase) ahead of FoldNull in the optimiser would also make sense -- maybe push these three to be the first rules?

I think that's a great idea! Should I just try it myself?

elasticsearchmachine · 2024-12-06T17:49:54Z

💚 Backport successful

Status	Branch	Result
✅	8.x

This reworks `Expressions#isNull` so it only matches the `null` literal. This doesn't super change the way ESQL works because we already rewrite things that `fold` into `null` into the `null` literal. It's just that, now, `isNull` won't return `true` for things that *fold* to null - only things that have *already* folded to null. This is important because `fold` can be quite expensive so we're better off keeping the results of it when possible. Which is what the constant folding rules *do*.

nik9000 added >non-issue :Analytics/Compute Engine Analytics in ES|QL v9.0.0 v8.18.0 labels Dec 5, 2024

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 5, 2024

idegtiarenko approved these changes Dec 6, 2024

View reviewed changes

astefan self-requested a review December 6, 2024 14:29

nik9000 commented Dec 6, 2024

View reviewed changes

costin approved these changes Dec 6, 2024

View reviewed changes

astefan approved these changes Dec 6, 2024

View reviewed changes

bpintea approved these changes Dec 6, 2024

View reviewed changes

nik9000 added the auto-backport Automatically create backport pull requests when merged label Dec 6, 2024

nik9000 merged commit 8c38007 into elastic:main Dec 6, 2024
16 checks passed

nik9000 mentioned this pull request Dec 6, 2024

[8.x] ESQL: Rework isNull (#118101) #118178

Merged

nik9000 mentioned this pull request Dec 13, 2024

ESQL: Limit memory usage of fold #118602

Merged

ESQL: Rework isNull #118101

ESQL: Rework isNull #118101

Uh oh!

Conversation

nik9000 commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 5, 2024

Uh oh!

nik9000 commented Dec 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Dec 6, 2024

Uh oh!

nik9000 commented Dec 6, 2024

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Dec 6, 2024

Uh oh!

Uh oh!

elasticsearchmachine commented Dec 6, 2024

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ESQL: Rework `isNull` #118101

ESQL: Rework `isNull` #118101

nik9000 commented Dec 5, 2024 •

edited

Loading