Skip to content

Replace hand-rolled output iterators with cuda::tabulate_output_iterator - #23856

Open
PointKernel wants to merge 1 commit into
NVIDIA:mainfrom
PointKernel:dedup-tabulate-output-iter
Open

Replace hand-rolled output iterators with cuda::tabulate_output_iterator#23856
PointKernel wants to merge 1 commit into
NVIDIA:mainfrom
PointKernel:dedup-tabulate-output-iter

Conversation

@PointKernel

@PointKernel PointKernel commented Aug 27, 2026

Copy link
Copy Markdown
Member

Description

There are a few places in cuDF that hand-roll output iterators where cuda::make_tabulate_output_iterator would do. This PR replaces them.

Each one reimplements the iterator internals just to write a single struct field, when all that is actually needed is the functor.

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 Aug 27, 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 Aug 27, 2026
@PointKernel

Copy link
Copy Markdown
Member Author

/ok to test

@PointKernel PointKernel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 27, 2026
@PointKernel
PointKernel marked this pull request as ready for review August 27, 2026 18:35
@PointKernel
PointKernel requested a review from a team as a code owner August 27, 2026 18:35
@coderabbitai

coderabbitai Bot commented Aug 27, 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: 20aa8227-a890-44a7-af95-9223019dcacf

📥 Commits

Reviewing files that changed from the base of the PR and between f588e0c and 4925c87.

📒 Files selected for processing (3)
  • cpp/src/copying/contiguous_split.cu
  • cpp/src/io/parquet/reader_impl_preprocess.cu
  • cpp/src/io/parquet/reader_impl_preprocess_utils.cuh

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


📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Streamlined GPU processing for contiguous data splitting and Parquet file reading.
    • Simplified internal handling of row counts, page offsets, and string offsets while preserving existing results.
    • No changes to public APIs or user-facing functionality.

Walkthrough

The change replaces custom Thrust output iterators with indexed device functors used by tabulate output iterators. Contiguous split and Parquet preprocessing retain their existing scan and reduction data flow.

Changes

CUDA output writes

Layer / File(s) Summary
Contiguous split output writes
cpp/src/copying/contiguous_split.cu
Partition offsets and valid counts now use tabulate output iterators with device functors. Scan and reduction behavior remains unchanged.
Parquet preprocessing output writes
cpp/src/io/parquet/reader_impl_preprocess_utils.cuh, cpp/src/io/parquet/reader_impl_preprocess.cu
Indexed functors write PageInfo::chunk_row and PageInfo::str_offset during list, subpass, and string-offset preprocessing scans.

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

Merge Risk: ⚪ Minimal · up to 4925c

This is a localized iterator implementation cleanup with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: nirandaperera, vyasr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains that the pull request replaces hand-rolled output iterators with cuda::make_tabulate_output_iterator.
Title check ✅ Passed The title accurately and concisely summarizes the main change: replacing hand-rolled output iterators with cuda::tabulate_output_iterator.
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.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@nirandaperera nirandaperera left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@bdice bdice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks fine but I vaguely recall there being an issue that forced us to create custom output iterators. Maybe a historical compiler bug or Thrust performance issue that is now resolved? Let's check the history before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function 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