fix: restrict array_filter array_compact fast path to the lambda variable#4848
Merged
andygrove merged 3 commits intoJul 13, 2026
Merged
Conversation
andygrove
force-pushed
the
fix-4830-array-filter-fast-path
branch
from
b6967a0 to
01d0b62
Compare
…able CometArrayFilter lowered filter(arr, x -> <IsNotNull>) to array_compact by only checking that the lambda body is an IsNotNull, not that its operand is the lambda's element variable. A body like x -> c IS NOT NULL (a captured column) was wrongly rewritten as array_compact, dropping the null elements of arr instead of Spark's semantics (keep all elements when c is non-null, empty array when c is null). Tighten the guard to require the IsNotNull operand to be the lambda's own NamedLambdaVariable; any other shape falls through to the codegen dispatcher. Closes apache#4830
andygrove
force-pushed
the
fix-4830-array-filter-fast-path
branch
from
01d0b62 to
59b1ca6
Compare
Contributor
|
Can we add tests for the following cases -
|
Contributor
|
Also, |
… guard Address review feedback on apache#4848: - Add SQL fixtures for four additional cases: IsNotNull on an expression of the lambda variable (x -> (x + 1) IS NOT NULL), a compound predicate (x -> x IS NOT NULL AND x > 0), a null-array input row, and an empty-array input row. Each exercises a distinct part of the guard or the fast path's null propagation. - Apply the same NamedLambdaVariable/exprId guard to the KnownNotContainsNull(ArrayFilter(IsNotNull)) branch in Spark4xCometExprShim so the Spark 4.x rewrite path rejects captured-column IsNotNull the same way the general serde does.
Member
Author
|
Thanks for the review @parthchandra. I have addressed feedback. Could you take another look? |
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
2 participants
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?
Closes #4830.
Rationale for this change
CometArrayFilter.converthas a fast path that lowersfilter(arr, x -> x IS NOT NULL)to the nativearray_compactserde, to avoid the per-batch JNI cost of the codegen dispatcher. The guard only checked that the lambda body is anIsNotNull, not that theIsNotNulloperand is the lambda's element variable.As a result, a lambda whose body is
IsNotNullof something other than the element (for example a captured column,x -> c IS NOT NULL) was incorrectly treated asarray_compact. Spark keeps every element ofarrwhencis non-null and returns an empty array whencis null; the fast path instead dropped the null elements ofarrregardless ofc. This produced silently wrong results wheneverarrcontained null elements or the captured column was null.What changes are included in this PR?
Tighten the guard in
CometArrayFilter.convertto fire thearray_compactfast path only when theIsNotNulloperand is the lambda's ownNamedLambdaVariable(matched byexprId, which also handles nested-lambda shadowing). Any other shape falls through to the codegen dispatcher, which runs Spark's own evaluation and matches Spark exactly.How are these changes tested?
Added regression queries to
array_filter.sql: a genuinearray_compactcase (x -> x IS NOT NULL) that must still fast-path, and the bug case (x -> c IS NOT NULLwith a captured column and null array elements) that must match Spark. Verified the captured-column case fails against the pre-fix code and passes with the fix, and that the genuine fast path still runs natively.