[common] Never compare null literals when merging range predicates - #9647
Open
LuciferYang wants to merge 1 commit into
Open
[common] Never compare null literals when merging range predicates#9647LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
close #9646
Between.optimizemerges<=and>=predicates on one field into aBETWEEN, and it ordered the two bounds without checking them:With a null bound that is a
NullPointerExceptionfromcompareTo(null), orRuntimeException("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 throughExpressionHelper.resolveFilter, which runsConstantFoldingbut notNullPropagation, 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.testtreats a null literal as no match. Merging into aBETWEENis an optimization, and skipping it changes nothing semantically.compareLiteralalso gained a guard, since it is the function whose contract was implicit: a null literal now gets anIllegalArgumentExceptionnaming the type instead of aNullPointerExceptionfrom insidecompareTo. 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(), sincek >= 1 AND k <= NULLreally does match nothing. I left it out:optimize's job is merging predicates, and having it start producingalwaysFalsereaches past whatPredicateBuilder.andasked for, while the evaluation layer already answers false for these.Tests
BetweenTest.testNullLiteralBoundsDoNotCrashbuildsk >= 1 AND k <= nullthroughPredicateBuilder.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 twoBETWEENs where one carries a null.Against the unfixed optimizer the test errors with a
NullPointerException.mvn -pl paimon-common -Dtest=BetweenTest,PredicateTest teston JDK 8: 50 tests, 0 failures.spotless:checkandcheckstyle:checkon paimon-common are clean.