fix: [branch-1.0] honour Spark's legacy null IN () behavior (#5127) - #5134
Merged
Merged
Conversation
* fix: honour Spark's legacy `null IN ()` behavior Comet's native `in` kernel always returns `false` for an operand against an empty `IN` list. Spark only does that under the non-legacy behavior; under the legacy behavior a `NULL` operand evaluates to `NULL` (SPARK-44550). That legacy behavior is in effect more often than the config name suggests: always on Spark 3.4 (no config), by default on Spark 3.5, and whenever ANSI mode is disabled on Spark 4.0+, where `spark.sql.legacy.nullInEmptyListBehavior` defaults to `!spark.sql.ansi.enabled`. Mark the empty-list case `Unsupported` in `CometIn` / `CometInSet` when the legacy behavior applies. Both serdes mix in `CodegenDispatchFallback`, so these cases run Spark's own `doGenCode` inside the Comet pipeline rather than falling the whole operator back to Spark. `CometNot`'s `Not(In(...))` fast path is excluded for the same case so it defers to `CometIn`. `OptimizeIn` folds empty `IN` lists away, so this only affects plans where that rule is excluded, and non-empty `IN` lists are unchanged. Closes apache#4786 * refactor: dedup the In/InSet support gates - Share the empty-list + collation gate between `CometIn` and `CometInSet` via `ComparisonUtils.inSupportLevel` / `inUnsupportedReasons` instead of two copies that must be edited in lockstep. - Read the legacy config through the existing `CometConf.getBooleanConf` helper rather than hand-rolling the string-key read, and make the resolver private. - Have `CometNot`'s fused fast paths consult the child serde's `getSupportLevel` instead of re-checking its conditions by hand, so gates added to `CometIn` or `CollationAwareBinaryPredicate` no longer need mirroring here. - Test: use `Rule.ruleName` for the excluded optimizer rules so a Spark rename cannot silently stop exercising the case. (cherry picked from commit 038fd6e)
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
1 participant
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.
Which issue does this PR close?
Cherry-pick of #5127 (merged to
mainas038fd6e25) ontobranch-1.0. Closes #4786 for the 1.0.0 release branch.Rationale for this change
Comet's native
inkernel always returnsfalsefor any operand against an emptyINlist. Spark only behaves that way under the non-legacy behavior; under the legacy behavior aNULLoperand evaluates toNULL(SPARK-44550), so Comet returnsfalsewhere Spark returnsNULL.That legacy behavior applies more often than the config name suggests:
In/InSetalways use the legacy behaviorspark.sql.legacy.nullInEmptyListBehaviordefaults totrue!spark.sql.ansi.enabled, so the legacy behavior is in effect whenever ANSI mode is disabledThis is a correctness divergence on every supported Spark version, so it is worth carrying into 1.0.0 rather than leaving it as a documented known issue.
What changes are included in this PR?
A straight
git cherry-pick -xof038fd6e25. The diff is byte-identical to the commit onmain; there were no conflicts and no adaptation was needed.CometIn/CometInSetreturnUnsupportedfor an empty list when the legacy behavior is in effect. Both serdes already mix inCodegenDispatchFallback, so these cases run Spark's owndoGenCodeinside the Comet pipeline instead of falling the whole operator back to Spark.CometNot'sNot(In(...))fast path skips the same case so it defers toCometIn(otherwiseNOT (null IN ())returnedtrueinstead ofNULL).branch-1.0by docs: [branch-1.0] document known correctness issues in the compatibility guide (#5085) #5117), and replacesCometIn's compatible note with an unsupported-case reason.OptimizeInrewrites emptyINlists away, so this only affects plans where that rule is excluded; non-emptyINlists are unaffected.How are these changes tested?
By the test added in #5127:
CometExpressionSuitecoversa IN ()andNOT (a IN ())over a Parquet scan with aNULLoperand, acrossspark.sql.legacy.nullInEmptyListBehaviorset totrue,false, and unset (which exercises the version- and ANSI-derived default) with ANSI mode both on and off.Verified locally on this branch under both
-Pspark-3.5and-Pspark-4.0; the test passes on each, and fails without the serde change.