Skip to content

Add memory resource parameter to cudf::key_remapping constructor - #23277

Merged
rapids-bot[bot] merged 8 commits into
NVIDIA:mainfrom
PointKernel:add-mr-key-remapping
Jul 27, 2026
Merged

Add memory resource parameter to cudf::key_remapping constructor#23277
rapids-bot[bot] merged 8 commits into
NVIDIA:mainfrom
PointKernel:add-mr-key-remapping

Conversation

@PointKernel

Copy link
Copy Markdown
Member

Description

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).

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

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 github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Jul 15, 2026
@PointKernel PointKernel added feature request New feature or request non-breaking Non-breaking change labels Jul 15, 2026
@PointKernel

Copy link
Copy Markdown
Member Author

/ok to test 85a48e3

@PointKernel
PointKernel marked this pull request as ready for review July 16, 2026 18:17
@PointKernel
PointKernel requested a review from a team as a code owner July 16, 2026 18:17
@PointKernel
PointKernel requested review from lamarrr and wence- July 16, 2026 18:17
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c5833720-7f68-43c0-949b-0dc840e20591

📥 Commits

Reviewing files that changed from the base of the PR and between 617b764 and b4db21e.

📒 Files selected for processing (1)
  • cpp/tests/join/key_remapping_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/tests/join/key_remapping_tests.cpp

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added support for selecting a custom device memory resource when creating key remapping operations.
    • Existing behavior remains unchanged when no memory resource is specified.
  • Tests

    • Added coverage validating custom memory resource usage and preserving expected key-matching results.

Walkthrough

The key_remapping constructor now accepts an optional device memory resource, propagates it to the internal CUCO hash table, and includes a test verifying allocation metrics and remapping behavior with an explicit resource.

Changes

Key remapping memory resource

Layer / File(s) Summary
Memory resource contract and allocator propagation
cpp/include/cudf/join/key_remapping.hpp, cpp/src/join/key_remapping.cu
Adds the optional memory resource parameter and forwards it through key remapping construction into the CUCO allocator.
Explicit resource validation
cpp/tests/join/key_remapping_tests.cpp
Adds a statistics-resource test that checks peak allocation usage and duplicate/distinct key remapping results.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: wence-, lamarrr

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a memory resource parameter to the key_remapping constructor.
Description check ✅ Passed The description is directly related to the changeset and accurately describes the new mr parameter and its purpose.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/tests/join/key_remapping_tests.cpp`:
- Around line 761-766: In the remap_right_keys test, add an assertion that ids
contains the expected number of rows before the indexed EXPECT_EQ and EXPECT_NE
checks. Keep the existing ID relationship assertions unchanged and ensure all
ids[0] through ids[3] accesses occur only after validating the output length.
🪄 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: 33a79d4d-4670-476d-98e2-ad9aae71443f

📥 Commits

Reviewing files that changed from the base of the PR and between 9d9fec8 and 617b764.

📒 Files selected for processing (3)
  • cpp/include/cudf/join/key_remapping.hpp
  • cpp/src/join/key_remapping.cu
  • cpp/tests/join/key_remapping_tests.cpp

Comment thread cpp/tests/join/key_remapping_tests.cpp
@vyasr

vyasr commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@PointKernel needs a style fix.

rapids-bot Bot pushed a commit that referenced this pull request Jul 20, 2026
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
@PointKernel

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit d14e9f5 into NVIDIA:main Jul 27, 2026
382 of 386 checks passed
@PointKernel
PointKernel deleted the add-mr-key-remapping branch July 27, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants