Repository navigation
fix(smj): compare all key types across batch boundaries in semi/anti/mark joins - #26830
Conversation
…mark joins keys_match still used compare_join_arrays for non-float keys, which rejects timezone-aware timestamps, Dictionary, Decimal256 and Time keys. Use JoinKeyComparator for every key type, as the merge scan does. Closes apache#26003.
github-actions
Bot
added
sqllogictest
|
@jayzhan211 @sunchao could one of you approve the CI run and take a look when you have time? It follows the approach of #25134. Thanks. |
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Thanks, dora. Reviewed d9922b4 with five independent passes covering correctness, datatype compatibility, performance/resource behavior, tests, and design. No actionable defects found.
I checked all four batch-boundary callers, including filtered buffering and spill replay. Reusing the scan's comparator keeps boundary equality consistent with within-batch grouping, including logical dictionary nulls and float normalization. The test helper preserves the existing float assertions, and the new matrix exercises all six semi/anti/mark joins with both null policies, both sort directions, and one-/two-row batches.
Verified the successful Rust fork CI run for this exact commit: all 240 new key-type cases and 144 existing float cases passed, and the SQL logic suite completed. My local rerun stopped before compilation because the configured registry could not resolve the locked Tokio version; no base/head performance benchmark was run.
|
thx sunchao, appreciate the review |
Which issue does this PR close?
Closes #26003.
Rationale for this change
EXISTS,INandNOT EXISTSqueries planned as sort-merge joins fail with "Unsupported data type in sort merge join comparator" on timezone-aware timestamp,Dictionary,Decimal256andTime32/Time64keys, once a key group runs to the end of a batch. Inner sort-merge join and hash join already work on these keys.What changes are included in this PR?
The bitwise stream compares keys with
JoinKeyComparatoreverywhere except inkeys_match, the check for whether a key group continues into the next batch. That function still calledcompare_join_arraysfor non-float keys, andcompare_join_arraysonly knows a fixed list of types. #25134 moved float keys over toJoinKeyComparator; this moves the rest.keys_matchnow builds a one-row comparator each time a key group reaches the end of a batch. The scan already rebuilds its comparators on every new batch, so I don't expect a measurable cost.One edge case changes:
compare_join_arraystreatedDataType::Nullkeys as always equal, whileJoinKeyComparatortreats them as unequal underNullEqualsNothing. As far as I can tell this can't change any output, because such keys never compare equal in the scan either, so the code paths that callkeys_matchdon't run for them.compare_join_arrayshas no callers left, but it's public injoins::utils, so I left it alone. Happy to deprecate it here or in a follow-up if you prefer.What is the testing strategy for this PR?
sort_merge_join.slt: the reproducer from the issue, plusbatch_size = 2queries forDictionary,Decimal256andTime64keys.join_key_type_batch_boundariesinsort_merge_join/tests.rs: the same matrix asjoin_float_key_batch_boundaries(join type, null equality, sort direction, 1- and 2-row batches) for the five key types above. I moved the body of the float test into a shared helper so the two don't duplicate it. Like fix(smj): handle floating-point keys across batch boundaries #25134, it's a physical-plan test, so batches split at exact rows regardless of what the optimizer does.Without the fix, all of the new cases fail with the error from the issue.
The change also covers other types
compare_join_arraysdidn't handle (e.g.Duration,Interval,Decimal32/Decimal64, nested types), but I haven't added tests for those.Are there any user-facing changes?
Those queries now return results instead of an error. No API changes.