[davidwendt]

Description

Several places in libcudf called release() on a std::unique_ptr and then moved from the raw pointer it returned (std::move(*ptr.release()), ptr.release()[0], or auto p = contents.data.release();). The device memory was moved out and freed normally, but nothing deleted the small host-side object (rmm::device_buffer, cuda::device_buffer<bitmask_type>, or cudf::column), so each call leaked it.

This PR moves from *ptr instead, so the unique_ptr keeps ownership and deletes the moved-from object.

It fixes all the sites listed in #24926, plus the same pattern found in:

• cpp/src/groupby/sort/group_argmin.cu, group_argmax.cu
• cpp/src/io/utilities/column_buffer.cpp
• java/src/main/native/src/ColumnViewJni.cu
• cpp/benchmarks/common/generate_input.cu (5 sites), cpp/benchmarks/io/text/multibyte_split.cpp

It also adds a one-line note about this pattern to the Resource Management section of cpp/REVIEW_GUIDELINES.md, so future reviews catch it.

The remaining unique_ptr::release() calls in the tree hand ownership off on purpose: the JNI ContiguousTableJni.cpp, DLPack/Arrow interop, and custom deleters.

Closes #24926

Checklist

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

🤖 Generated with Claude Code

[copy-pr-bot]

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.

[davidwendt]

/ok to test

[davidwendt]

@coderabbitai full review

[coderabbitai]

Action performed

Full review finished.

[coderabbitai]

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

• Refactor
  • Updated resource transfers across column, string, dictionary, and reduction operations to preserve ownership when moving data and masks.
  • Clarified resource-management guidance on safely moving values from smart pointers.

The changes replace raw-pointer releases with moves from the objects owned by std::unique_ptr across column construction, string processing, reductions, and benchmark code. The resource-management guideline documents this distinction.

Changes: Unique-pointer move ownership

Layer / File(s)

Summary

Move objects without releasing their owners cpp/REVIEW_GUIDELINES.md, cpp/benchmarks/common/generate_input.cu, cpp/benchmarks/io/text/multibyte_split.cpp, cpp/src/copying/copy_range.cu, cpp/src/dictionary/..., cpp/src/interop/from_arrow_host_strings.cu, cpp/src/io/json/write_json.cu, cpp/src/io/utilities/column_buffer.cpp, cpp/src/lists/contains.cu, cpp/src/reductions/..., cpp/src/strings/replace/*, cpp/src/transform/nans_to_nulls.cu, java/src/main/native/src/ColumnViewJni.cu

Column, buffer, and null-mask values are moved from their owning pointers instead of releasing those pointers first. The benchmark changes apply the same pattern. The guideline warns that moving from a pointer returned by std::unique_ptr::release() leaks the host object.

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

Change: Bug fix

Suggested reviewers: vyasr, abellina

Merge Risk: Minimal · up to ceb02

No merge-blocking issue was found in the ownership-transfer changes.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title names the main change: moving from the object behind a unique_ptr instead of from the raw pointer returned by release(), which fixes leaked host objects. The changeset matches this, since it…
Description check		The description is related to the changeset. It explains the leak caused by moving from the pointer returned by release(), says the fix moves from *ptr so the unique_ptr deletes the moved-from object,…
Linked Issues check		The only active directly linked target is #24926. Its requirements are the listed sites in Forms 1, 2, and 3. The whole-PR summary (merge base to head) shows each listed site changed to keep ownership…
Out of Scope Changes check		The changes in the whole-PR summary map to the #24926 leak pattern or to the documentation of that pattern. These include the 17 source and benchmark files listed in the summary (copy_range, concatena…

Full details: Title check

Explanation

The title names the main change: moving from the object behind a unique_ptr instead of from the raw pointer returned by release(), which fixes leaked host objects. The changeset matches this, since it replaces release()-then-move patterns across many libcudf files and adds a guideline note. The title is specific and accurate.

Full details: Description check

Explanation

The description is related to the changeset. It explains the leak caused by moving from the pointer returned by release(), says the fix moves from *ptr so the unique_ptr deletes the moved-from object, lists the affected sites, and notes the added REVIEW_GUIDELINES.md note. It is detailed and on-topic.

Full details: Linked Issues check

Explanation

The only active directly linked target is #24926. Its requirements are the listed sites in Forms 1, 2, and 3. The whole-PR summary (merge base to head) shows each listed site changed to keep ownership in the unique_ptr and move from the pointed-to object: copying/copy_range.cu (2 sites), dictionary/detail/concatenate.cu, dictionary/dictionary_factories.cu (2 sites), interop/from_arrow_host_strings.cu, lists/contains.cu, reductions/histogram.cu, reductions/segmented/simple.cuh (2 sites), transform/nans_to_nulls.cu (2 sites), io/json/write_json.cu (2 sites), strings/replace/multi.cu, strings/replace/replace.cu, and reductions/collect_ops.cu. The collect_ops.cu diff at head confirms the requested fix: 'auto null_purged_col = std::move(null_purged_table->release().front());', with later uses of null_purged_col unchanged. The issue states no separate automated-test requirement. The PR description also names cpp/src/groupby/sort/group_argmin.cu and group_argmax.cu as fixed. Those files do not exist at the reviewed head, and a whole-PR diff for them is empty. This is a description inaccuracy. It does not affect #24926, which does not list those files. The changes since the previous review were not itemized, so the verdict relies on the whole-PR summary and the collect_ops.cu diff. The summary supports compliance with all listed #24926 objectives.

Full details: Out of Scope Changes check

Explanation

The changes in the whole-PR summary map to the #24926 leak pattern or to the documentation of that pattern. These include the 17 source and benchmark files listed in the summary (copy_range, concatenate, dictionary_factories, from_arrow_host_strings, write_json, column_buffer, lists/contains, collect_ops, histogram, simple.cuh, multi, replace, nans_to_nulls, generate_input, multibyte_split, ColumnViewJni) and the one-line note in cpp/REVIEW_GUIDELINES.md. Each change moves from a dereferenced unique_ptr instead of a released raw pointer and does not change control flow or values. The changes listed in the summary do not add unrelated behavior. The PR description's claim of groupby changes has no matching diff. The absence of those changes does not create out-of-scope edits.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 14 functions across 15 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Recent review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/cudf
• Branch: main
• File: .coderabbit.yaml
• Commit: 0765b8f
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 3b18a28b-a409-41db-b66c-0a0e8097b19b

Commits

Reviewing files that changed from the base of the PR and between 70f89c2 and ceb02ca.

Files selected for processing: 4

• cpp/benchmarks/common/generate_input.cu
• cpp/src/reductions/segmented/simple.cuh
• cpp/src/strings/replace/multi.cu
• cpp/src/strings/replace/replace.cu

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

[davidwendt]

/merge