fix: allow NULL-padded rows in sort-merge join filter evaluation - #26027
Open
CuteChuanChuan wants to merge 2 commits into
Open
CuteChuanChuan wants to merge 2 commits into
CuteChuanChuan wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26027 +/- ##
==========================================
- Coverage 82.65% 82.65% -0.01%
==========================================
Files 1147 1147
Lines 446087 446107 +20
Branches 446087 446107 +20
==========================================
+ Hits 368710 368720 +10
- Misses 54990 54996 +6
- Partials 22387 22391 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Hi @jayzhan211 , could you PTAL when you have a chance? Thanks 🙏! |
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.
Which issue does this PR close?
Rationale for this change
With sort-merge join (
datafusion.optimizer.prefer_hash_join = false), aLEFT,RIGHTorFULLjoin with an extra join filter fails withArrow error: Invalid argument error: Column '...' is declared as non-nullable but contains null valueswhen the NULL-padded side hasNOT NULLcolumns referenced by the filter. Hash join returns the correct result for the same queries.What changes are included in this PR?
Root cause. As @jayzhan211 pointed out in the issue, when a streamed row has no match but the buffered side still holds a batch,
null_join_streamed_rowappends it withSome(scanning_batch_idx), so it is materialized in the same chunk as matched rows. Its NULL-padded buffered-side columns then reach join filter evaluation, but the filter's intermediate schema declares them non-nullable, soRecordBatch::try_newfails.Fix. Following the fix suggested in the issue,
MaterializingSortMergeJoinStream::try_newbuilds the filter schema once with buffered-side fields marked nullable (JoinSide::RightforLeft/Full,JoinSide::LeftforRight), and filter evaluation uses it. It is only built whendeferred_filteringis set (outer join with a filter); inner joins keep the original schema.Why this is safe. The filter result for a NULL-padded row does not affect the output. Such a row is the only entry for its streamed row, so
get_corrected_filter_maskkeeps it as-is if the filter returns true and null-joins it if false; either way the same NULL-padded row is emitted. TheFulljoin filter-status tracking already skips NULL buffered indices, so this path already expects such rows.Alternative considered. Keep NULL-padded rows out of the filter by appending them with
buffered_batch_idx = None. Done naively, this reorders output:freeze_streamedpushesNonechunks immediately but pushes matched chunks together at the end, so sort order would be silently broken. Flushing matched chunks before eachNonechunk would preserve order, but splits the single batched filter evaluation infreeze_streamed_matched, which matters most when keys are near-unique. Making the schema nullable avoids both.What is the testing strategy for this PR?
Added sqllogictest cases to
sort_merge_join.sltwithNOT NULLcolumns andtarget_partitions = 2(needed to reproduce; the queries pass with 1 partition):LEFT,FULLandRIGHTjoins from the issue, with expected results matching hash join.LEFTjoin whose filter divides by a buffered-side column (a.v / b.w < 1), to check that the zero-valued slots under NULL-padded rows do not cause a divide-by-zero error.All four queries fail on
mainwith the Arrow error above and pass with this change. The full extended test suite also passes locally.Are there any user-facing changes?
No. Bug fix only; no public API change.