-
Notifications
You must be signed in to change notification settings - Fork 25.8k
ESQL: Rework isNull
#118101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESQL: Rework isNull
#118101
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,14 +151,14 @@ public Expression replaceChildren(List<Expression> newChildren) { | |
| public boolean foldable() { | ||
| // QL's In fold()s to null, if value() is null, but isn't foldable() unless all children are | ||
| // TODO: update this null check in QL too? | ||
| return Expressions.isNull(value) | ||
| return Expressions.isGuaranteedNull(value) | ||
| || Expressions.foldable(children()) | ||
| || (Expressions.foldable(list) && list.stream().allMatch(Expressions::isNull)); | ||
| || (Expressions.foldable(list) && list.stream().allMatch(Expressions::isGuaranteedNull)); | ||
| } | ||
|
|
||
| @Override | ||
| public Object fold() { | ||
| if (Expressions.isNull(value) || list.stream().allMatch(Expressions::isNull)) { | ||
| if (Expressions.isGuaranteedNull(value) || list.stream().allMatch(Expressions::isGuaranteedNull)) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about this one. I feel like it's tricky to be sure this is right.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to try removing this line from ESQL entirely to see what happens.... Learning!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return null; | ||
| } | ||
| return super.fold(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,21 +30,21 @@ public Expression rule(Expression e) { | |
| // perform this early to prevent the rule from converting the null filter into nullifying the whole expression | ||
| // 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())) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return agg.withFilter(Literal.of(agg.filter(), false)); | ||
| } | ||
| } | ||
|
|
||
| if (result != e) { | ||
| return result; | ||
| } else if (e instanceof In in) { | ||
| if (Expressions.isNull(in.value())) { | ||
| if (Expressions.isGuaranteedNull(in.value())) { | ||
| return Literal.of(in, null); | ||
| } | ||
| } else if (e instanceof Alias == false | ||
| && e.nullable() == Nullability.TRUE | ||
| && e instanceof Categorize == false | ||
| && Expressions.anyMatch(e.children(), Expressions::isNull)) { | ||
| && Expressions.anyMatch(e.children(), Expressions::isGuaranteedNull)) { | ||
| return Literal.of(e, null); | ||
| } | ||
| return e; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,7 @@ protected LogicalPlan rule(Filter filter) { | |
| if (TRUE.equals(condition)) { | ||
| return filter.child(); | ||
| } | ||
| if (FALSE.equals(condition) || Expressions.isNull(condition)) { | ||
| if (FALSE.equals(condition) || Expressions.isGuaranteedNull(condition)) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And that someone could be me.... |
||
| return PruneEmptyPlans.skipPlan(filter); | ||
| } | ||
| } | ||
|
|
@@ -42,8 +42,8 @@ protected LogicalPlan rule(Filter filter) { | |
|
|
||
| private static Expression foldBinaryLogic(BinaryLogic binaryLogic) { | ||
| if (binaryLogic instanceof Or or) { | ||
| boolean nullLeft = Expressions.isNull(or.left()); | ||
| boolean nullRight = Expressions.isNull(or.right()); | ||
| boolean nullLeft = Expressions.isGuaranteedNull(or.left()); | ||
| boolean nullRight = Expressions.isGuaranteedNull(or.right()); | ||
| if (nullLeft && nullRight) { | ||
| return new Literal(binaryLogic.source(), null, DataType.NULL); | ||
| } | ||
|
|
@@ -55,7 +55,7 @@ private static Expression foldBinaryLogic(BinaryLogic binaryLogic) { | |
| } | ||
| } | ||
| if (binaryLogic instanceof And and) { | ||
| if (Expressions.isNull(and.left()) || Expressions.isNull(and.right())) { | ||
| if (Expressions.isGuaranteedNull(and.left()) || Expressions.isGuaranteedNull(and.right())) { | ||
| return new Literal(binaryLogic.source(), null, DataType.NULL); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
isGuaranteedNullmakes me wonder what is "null" and more relevant when is it not guaranteed :)I'd find something like
isNullTypeOrLiteralclearer, but just a nitty nit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I think I'm going to keep it because everything feels not so great. Maybe
valueIsGuaranteedNullor something? I'm not sure that makes it more descriptive.