Skip to content

Commit

Permalink
ESQL: Rework isNull
Browse files Browse the repository at this point in the history
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*.
  • Loading branch information
nik9000 committed Dec 5, 2024
1 parent 422eb1a commit 2e0ef03
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,16 @@ public static String name(Expression e) {
return e instanceof NamedExpression ne ? ne.name() : e.sourceText();
}

public static boolean isNull(Expression e) {
return e.dataType() == DataType.NULL || (e.foldable() && e.fold() == null);
/**
* Is this {@linkplain Expression} <strong>guaranteed</strong> to have
* only the {@code null} value. {@linkplain Expression}s that
* {@link Expression#fold()} to {@code null} <strong>may</strong>
* return {@code false} here, but should <strong>eventually</strong> be folded
* into a {@link Literal} containing {@code null} which will return
* {@code true} from here.
*/
public static boolean isGuaranteedNull(Expression e) {
return e.dataType() == DataType.NULL || (e instanceof Literal lit && lit.value() == null);
}

public static List<String> names(Collection<? extends Expression> e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
return null;
}
return super.fold();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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())) {
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
return PruneEmptyPlans.skipPlan(filter);
}
}
Expand All @@ -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);
}
Expand All @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public Expression rule(In in) {
List<Expression> foldables = new ArrayList<>(in.list().size());
List<Expression> nonFoldables = new ArrayList<>(in.list().size());
in.list().forEach(e -> {
if (e.foldable() && Expressions.isNull(e) == false) { // keep `null`s, needed for the 3VL
if (e.foldable() && Expressions.isGuaranteedNull(e) == false) { // keep `null`s, needed for the 3VL
foldables.add(e);
} else {
nonFoldables.add(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4820,7 +4820,7 @@ private static boolean oneLeaveIsNull(Expression e) {

e.forEachUp(node -> {
if (node.children().size() == 0) {
result.set(result.get() || Expressions.isNull(node));
result.set(result.get() || Expressions.isGuaranteedNull(node));
}
});

Expand Down

0 comments on commit 2e0ef03

Please sign in to comment.