Add a direct_inner_join API for pre-hashed distinct UINT32 keys - #23147
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. |
|
Benchmark sweep of the new |
|
/ok to test 29e7764 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new ChangesDirect inner join feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/tests/join/direct_join_tests.cpp (1)
60-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a sliced-column test case.
Current tests cover dense/sparse/empty/invalid-type/null/capacity cases well, but none exercise a sliced
column_view(non-zero offset) forleft_keys/right_keys, which the coding guidelines call out as an edge case worth covering for join-style APIs.🤖 Prompt for 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. In `@cpp/tests/join/direct_join_tests.cpp` around lines 60 - 79, Add a join test that uses a sliced column_view with a non-zero offset for either left_keys or right_keys in DirectJoinTest. Extend the DirectJoinTest coverage alongside DenseKeys and SparseKeys so compare_to_reference is exercised with a sliced input, ensuring the join path handles non-zero column offsets correctly.Source: Coding guidelines
🤖 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/src/join/direct_join.cu`:
- Around line 93-96: The temporary lookup buffer in direct_join.cu is being
allocated with the default RMM resource instead of cuDF’s tracked resource.
Update the lookup allocation in the direct join path to pass
cudf::get_current_device_resource_ref() when constructing the
rmm::device_uvector<size_type> in the block that builds the lookup table,
keeping the rest of the DeviceTransform::Fill logic unchanged.
In `@cpp/tests/streams/join_test.cpp`:
- Around line 90-95: The DirectInnerJoin test is using an out-of-contract key
because direct_inner_join is called with capacity 4 while left_keys includes 5,
which can index past the lookup table. Update JoinTest.DirectInnerJoin so the
keys stay within [0, capacity) or increase the capacity to cover the largest
key, keeping the test data consistent with the direct_inner_join contract.
---
Nitpick comments:
In `@cpp/tests/join/direct_join_tests.cpp`:
- Around line 60-79: Add a join test that uses a sliced column_view with a
non-zero offset for either left_keys or right_keys in DirectJoinTest. Extend the
DirectJoinTest coverage alongside DenseKeys and SparseKeys so
compare_to_reference is exercised with a sliced input, ensuring the join path
handles non-zero column offsets correctly.
🪄 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: 3fad77ec-2416-44d9-9a06-600dfe06b158
📒 Files selected for processing (8)
cpp/CMakeLists.txtcpp/benchmarks/CMakeLists.txtcpp/benchmarks/join/direct_join.cucpp/include/cudf/join/direct_join.hppcpp/src/join/direct_join.cucpp/tests/CMakeLists.txtcpp/tests/join/direct_join_tests.cppcpp/tests/streams/join_test.cpp
# Conflicts: # cpp/benchmarks/CMakeLists.txt
|
@shrshi and @kingcrimsontianyu would you please share your review? |
kingcrimsontianyu
left a comment
There was a problem hiding this comment.
Looks great. Left a minor question.
shrshi
left a comment
There was a problem hiding this comment.
This is awesome! Very clean implementation :)
|
/merge |
…dsai#23147) Closes rapidsai#23146 This PR adds a `direct_inner_join` free function to libcudf, the first step of the perfect hash join effort in rapidsai#23126. The keys are a single `UINT32` column per side, produced by a prior perfect hashing pass such as `cudf::key_remapping`, dictionary encoding, or dense integer primary keys. The right keys act as a perfect hash of the right rows: a lookup table of caller-specified `capacity` entries maps each key value to its row index and each left key probes that table directly, so the join performs no hashing or key comparison at all. - The caller controls the memory footprint via the explicit `capacity` argument. All key values must be in `[0, capacity)` and the right keys must be distinct; behavior is undefined otherwise. - Inner join only, as a free function: no table reuse across probes is needed, so there is no join object. - The build scatters right row indices into the lookup table with `cub::DeviceTransform::Fill` + `cub::DeviceFor::Bulk`; the probe is a single `cub`-based `copy_if` pass emitting the matched index pairs. - A new `JOIN_NVBENCH` benchmark compares `inner_join`, `distinct_hash_join`, and `direct_inner_join` on identical conforming input; results in the comment below. Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - Tianyu Liu (https://github.com/kingcrimsontianyu) - Shruti Shivakumar (https://github.com/shrshi) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: rapidsai#23147
Description
Closes #23146
This PR adds a
direct_inner_joinfree function to libcudf, the first step of the perfect hash join effort in #23126. The keys are a singleUINT32column per side, produced by a prior perfect hashing pass such ascudf::key_remapping, dictionary encoding, or dense integer primary keys. The right keys act as a perfect hash of the right rows: a lookup table of caller-specifiedcapacityentries maps each key value to its row index and each left key probes that table directly, so the join performs no hashing or key comparison at all.capacityargument. All key values must be in[0, capacity)and the right keys must be distinct; behavior is undefined otherwise.cub::DeviceTransform::Fill+cub::DeviceFor::Bulk; the probe is a singlecub-basedcopy_ifpass emitting the matched index pairs.JOIN_NVBENCHbenchmark comparesinner_join,distinct_hash_join, anddirect_inner_joinon identical conforming input; results in the comment below.Checklist