Add memory resource parameter to cudf::hash_join constructors - #23223
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. |
|
/ok to test 8e12597 |
|
|
||
| /** | ||
| * @copydoc hash_join(cudf::table_view const&, null_equality, rmm::cuda_stream_view) | ||
| * @brief Construct a hash join object for subsequent probe calls. |
There was a problem hiding this comment.
copydoc causes Doxygen check failures, and the only way to fix them is to duplicate the documentation instead of using copydoc.
|
/ok to test c2c5d7d |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughHash join constructors now accept an explicit device memory resource, propagate it to internal hash-table allocations, and retain default-resource behavior for public overloads. A test verifies allocation tracking and inner-join results with a statistics resource adaptor. ChangesHash Join Memory Resource
Estimated code review effort: 3 (Moderate) | ~20 minutes 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: 1
🤖 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/detail/join/hash_join.hpp`:
- Around line 71-82: Replace the `@copydoc` directive above the detail-header
hash_join overload with duplicated documentation matching the corresponding
public declaration, including its parameter descriptions and the existing
load_factor documentation. Update only the documentation for hash_join; retain
the declaration and signatures unchanged.
🪄 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: 09118cc4-427a-46fb-974e-790095f0c448
📒 Files selected for processing (4)
cpp/include/cudf/detail/join/hash_join.hppcpp/include/cudf/join/hash_join.hppcpp/src/join/hash_join/hash_join.cucpp/tests/join/join_tests.cpp
There was a problem hiding this comment.
This seems almost-right. We're using the passed-in mr because this is object-oriented, so we're treating the hash table more like output memory than temporary memory. However, this also needs to store a cuda::mr::any_resource<device_accessible> as a member of the cudf::hash_join object -- we don't want to put requirements on the lifetime of a ref. The resource should be reified and kept alive by the object holding the memory.
Compare to rmm::device_buffer, because this class is now more like a data container. https://github.com/rapidsai/rmm/blob/main/cpp/include/rmm/device_buffer.hpp
For container-like classes, we use cuda::mr::any_resource<cuda::mr::device_accessible> as the mr parameter rather than a ref, to allow construction from resource rvalues to avoid making a copy. Then store _mr{std::move(mr)} as a member.
|
@bdice Good point on ownership. Rather than adding a member, I switched the constructor |
bdice
left a comment
There was a problem hiding this comment.
Excellent, thanks for the ownership fix!
|
/merge |
#23263) Follow-up to #23220 and #23223. This PR adds an `mr` parameter to the `cudf::distinct_hash_join` constructors to control the memory usage of the internal cuco hash table, to better align with the temporary mr effort (#20780). Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - Muhammad Haseeb (https://github.com/mhaseeb123) - Vyas Ramasubramani (https://github.com/vyasr) URL: #23263
…3264) Follow-up to #23220 and #23223. This PR adds an `mr` parameter to the `cudf::filtered_join` constructors to control the memory usage of the internal cuco hash table, to better align with the temporary mr effort (#20780). Authors: - Yunsong Wang (https://github.com/PointKernel) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - David Wendt (https://github.com/davidwendt) - Vyas Ramasubramani (https://github.com/vyasr) URL: #23264
Description
Follow-up to #23220.
This PR adds an
mrparameter to thecudf::hash_joinconstructors to control the memory usage of the internal cuco data structures, to better align with the temporary mr effort (#20780). The resource must remain valid for the lifetime of thehash_joinobject.Checklist