Add memory resource parameter to cudf::filtered_join constructors - #23264
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 78d0628 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughFiltered join constructors now accept a device memory resource, propagate it to internal hash-table allocation, and retain a default public resource. A semi-join test verifies tracked allocation and expected results. ChangesFiltered 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 |
Follow-up to #23264 and #23277. This PR adds an `mr` parameter to the `cudf::mark_join` constructors to control allocations for the persistent cuco hash table and optional bloom-filter prefilter, aligning the wrapper with the temporary MR effort (#20780). Authors: - Yunsong Wang (https://github.com/PointKernel) Approvers: - David Wendt (https://github.com/davidwendt) - Qi Chen (https://github.com/qbacpey) URL: #23278
|
/merge |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tests/join/semi_anti_join_tests.cpp (1)
550-560: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExercise
cudf::filtered_join, notcudf::mark_join.Lines 557-560 only validate memory allocation for
cudf::mark_join; they never constructcudf::filtered_joinor callsemi_join/anti_join. Consequently, this test will pass even if the new filtered-join constructor dropsmrbefore allocating its internal hash table. Constructcudf::filtered_joinwithmr, execute a filtered join, and assert the tracked allocation plus expected row indices.🤖 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/semi_anti_join_tests.cpp` around lines 550 - 560, Update MarkJoinMemoryResource to construct cudf::filtered_join with mr and execute a semi_join or anti_join using the test tables. Assert mr records a positive allocation and verify the returned row indices match the expected filtered-join result, rather than testing cudf::mark_join construction alone.
🤖 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.
Outside diff comments:
In `@cpp/tests/join/semi_anti_join_tests.cpp`:
- Around line 550-560: Update MarkJoinMemoryResource to construct
cudf::filtered_join with mr and execute a semi_join or anti_join using the test
tables. Assert mr records a positive allocation and verify the returned row
indices match the expected filtered-join result, rather than testing
cudf::mark_join construction alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 17f8b66d-d12a-495b-ae27-9ed442be23c2
📒 Files selected for processing (1)
cpp/tests/join/semi_anti_join_tests.cpp
) Follow-up to #23264. This PR adds an `mr` parameter to the `cudf::key_remapping` constructor 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: - Vyas Ramasubramani (https://github.com/vyasr) - Basit Ayantunde (https://github.com/lamarrr) URL: #23277
Description
Follow-up to #23220 and #23223.
This PR adds an
mrparameter to thecudf::filtered_joinconstructors to control the memory usage of the internal cuco hash table, to better align with the temporary mr effort (#20780).Checklist