Skip to content

[common] Never compare null literals when merging range predicates - #9647

Open
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/between-null-literal
Open

[common] Never compare null literals when merging range predicates#9647
LuciferYang wants to merge 1 commit into
apache:masterfrom
LuciferYang:fix/between-null-literal

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Purpose

close #9646

Between.optimize merges <= and >= predicates on one field into a BETWEEN, and it ordered the two bounds without checking them:

Object lowerBound = greaterOrEqual.literals().get(0);
Object upperBound = lessOrEqual.literals().get(0);
if (compareLiteral(type, lowerBound, upperBound) >= 0) {

With a null bound that is a NullPointerException from compareTo(null), or RuntimeException("Unsupported type") when the null is the first argument. CALL sys.compact(table => 'db.t', where => 'dt >= 1 AND dt <= null') hits it: procedure filters go through ExpressionHelper.resolveFilter, which runs ConstantFolding but not NullPropagation, so the null literal reaches Paimon intact.

A null bound has no order, so the pair is now left unmerged and evaluated by the rules that already handle this: LeafBinaryFunction.test treats a null literal as no match. Merging into a BETWEEN is an optimization, and skipping it changes nothing semantically.

compareLiteral also gained a guard, since it is the function whose contract was implicit: a null literal now gets an IllegalArgumentException naming the type instead of a NullPointerException from inside compareTo. Its other callers (NotIn, NotBetween, LessThan) either null-check first or are only reached with non-null bounds, so nothing else changes.

I did consider short-circuiting the merge to alwaysFalse(), since k >= 1 AND k <= NULL really does match nothing. I left it out: optimize's job is merging predicates, and having it start producing alwaysFalse reaches past what PredicateBuilder.and asked for, while the evaluation layer already answers false for these.

Tests

BetweenTest.testNullLiteralBoundsDoNotCrash builds k >= 1 AND k <= null through PredicateBuilder.and, asserts it does not throw, and asserts the merged predicate evaluates to false for a row. It also covers a null lower bound and two BETWEENs where one carries a null.

Against the unfixed optimizer the test errors with a NullPointerException.

mvn -pl paimon-common -Dtest=BetweenTest,PredicateTest test on JDK 8: 50 tests, 0 failures. spotless:check and checkstyle:check on paimon-common are clean.

Between.optimize ordered the two bounds it merges into a BETWEEN with
compareLiteral, which throws NullPointerException on a null literal. A
Spark procedure where-clause can carry one, since resolveFilter does not
run NullPropagation.

Leave a pair with a null bound unmerged and let the evaluation layer
answer false for it, as LeafBinaryFunction already does, and guard
compareLiteral so the next caller gets a message instead of an NPE.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] A null literal in a procedure where-clause NPEs while merging range predicates

1 participant