Rewrite mixed inner/left/full join with post-filtering - #23012
Merged
rapids-bot[bot] merged 23 commits intoJul 22, 2026
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
Bot
added
libcudf
PointKernel
added
improvement
Member
Author
|
/ok to test edd3b59 |
Member
Author
|
/ok to test 130ffd2 |
Member
Author
|
/ok to test e9cc88d |
Member
Author
|
/ok to test f2705ac |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces bespoke mixed-join execution with ChangesMixed Join Refactor via filter_join_indices
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cudf/join/join.hpp`:
- Around line 425-434: The public API change in filter_join_indices_output_size
is source-breaking because it alters both the return type and the signature in a
public header. Preserve the existing contract by keeping the current
filter_join_indices_output_size declaration as a deprecated wrapper, or
introduce the new behavior under a different name and have the old function
forward to it. Make sure the symbols filter_join_indices_output_size,
cudf::table_view, and the join_kind-based overload remain available for
downstream callers during the transition.
- Around line 372-390: The public filter_join_indices overload currently trusts
output_size, which can lead to out-of-bounds writes or unsafe size arithmetic in
the implementation. Update filter_join_indices to validate the hint before use,
especially for the INNER_JOIN path and the LEFT/FULL size calculations, and fail
cleanly on mismatch rather than proceeding with unchecked allocation/copy
bounds. If this contract cannot be enforced safely, keep the size-hinting fast
path internal instead of exposing it in the public API.
In `@cpp/tests/join/mixed_join_tests.cu`:
- Around line 274-281: The mixed join test is only checking the sum of
`actual_counts`, which can miss incorrect per-row count placement; keep the
existing total check in `mixed_join_tests.cu`, but also validate `actual_counts`
element-wise against `expected_counts` in the test that uses `thrust::reduce`
and `result_size`. Update the assertions around the `actual_counts` vector so
the contract for output counts is verified entry-by-entry, not just by total.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9c54aefe-bb98-416a-b078-7748b1a1a542
📒 Files selected for processing (23)
cpp/CMakeLists.txtcpp/include/cudf/detail/join/join.hppcpp/include/cudf/join/join.hppcpp/src/join/filter_join_indices/filter_join_indices.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel.cuhcpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel.hppcpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_complex.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_null_complex.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_null_primitive.cucpp/src/join/filter_join_indices/filter_join_indices_output_size_kernel_primitive.cucpp/src/join/mixed_filter_join_common_utils.cuhcpp/src/join/mixed_join.cucpp/src/join/mixed_join_common_utils.cuhcpp/src/join/mixed_join_kernel.cucpp/src/join/mixed_join_kernel.cuhcpp/src/join/mixed_join_kernel.hppcpp/src/join/mixed_join_kernel_nulls.cucpp/src/join/mixed_join_semi.cucpp/src/join/mixed_join_size_kernel.cucpp/src/join/mixed_join_size_kernel.cuhcpp/src/join/mixed_join_size_kernel.hppcpp/src/join/mixed_join_size_kernel_nulls.cucpp/tests/join/mixed_join_tests.cu
💤 Files with no reviewable changes (9)
- cpp/src/join/mixed_join_kernel.hpp
- cpp/src/join/mixed_join_size_kernel.hpp
- cpp/src/join/mixed_join_size_kernel.cu
- cpp/src/join/mixed_join_size_kernel.cuh
- cpp/src/join/mixed_join_kernel.cuh
- cpp/src/join/mixed_join_kernel.cu
- cpp/src/join/mixed_join_size_kernel_nulls.cu
- cpp/CMakeLists.txt
- cpp/src/join/mixed_join_kernel_nulls.cu
Member
Author
|
Performance impact of the post-filter mixed join. The slowdown is significant for small input sizes. In contrast, for large datasets, the optimization improves performance by several milliseconds. |
shrshi
reviewed
Jul 16, 2026
shrshi
left a comment
shrshi
left a comment
Contributor
There was a problem hiding this comment.
Looks great overall! Some clarifying questions:
| * @param mr Device memory resource used to allocate the returned contribution counts. | ||
| * | ||
| * @return The exact number of pairs that `filter_join_indices` would produce. | ||
| * @return A pair containing the exact number of pairs that `filter_join_indices` would produce |
Contributor
There was a problem hiding this comment.
Two questions here: (i) Can you help me understand why we need the per-output contribution vector, and (ii) Is it binary vector to indicate which rows are present in the filtered output? Should we add the comment in filter_join_indices_output_size_kernel here as well?
Member
Author
There was a problem hiding this comment.
(i) why we need the per-output contribution vector
Good question! It's only there to keep the existing mixed-join two-pass interface source-compatible:
The legacy kernel used the per-row counts to compute write offsets in the retrieve pass; this filter-based rewrite doesn't, detail::mixed_join reads only the scalar output_size_data->first:
I'll check whether Spark actually reads the vector as their early chunked-probing attempt. If nobody does, I'll file a follow-up to deprecate it and return the total only; if chunked probing is genuinely wanted, the proper fix is the match-context design you introduced, shared across joins, rather than this passthrough:
(ii) Should we add the comment in filter_join_indices_output_size_kernel here as well?
Done, added the per-join-kind layout to this doc.
mhaseeb123
approved these changes
Jul 17, 2026
mhaseeb123
left a comment
mhaseeb123
left a comment
Contributor
There was a problem hiding this comment.
Approving CMake only.
shrshi
approved these changes
Jul 17, 2026
…-via-filter-indices
ttnghia
reviewed
Jul 21, 2026
ttnghia
approved these changes
Jul 21, 2026
Member
Author
|
/merge |
rapids-bot
Bot
merged commit Jul 22, 2026
7762df3
into
NVIDIA:release/26.08
259 of 261 checks passed
This was referenced Jul 22, 2026
This was referenced Jul 30, 2026
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
8 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.
Description
Part of #22124
This PR rewrites the mixed inner/left/full joins as equality-then-filter: run the keys through
cudf::hash_join, then apply the conditional predicate to the index pairs via a newfilter_join_indicesprimitive (AST + JIT, plusfilter_join_indices_output_size).mixed_full_joinbecomes a left join plusfinalize_full_join.This replaces the fused mixed-join kernels, reusing
hash_joininstead of duplicating it.Checklist