Skip to content

Add hybrid scan multifile two-step materializers - #22833

Merged
rapids-bot[bot] merged 45 commits into
NVIDIA:mainfrom
mhaseeb123:fea/two-step-materialize
Jun 29, 2026
Merged

Add hybrid scan multifile two-step materializers#22833
rapids-bot[bot] merged 45 commits into
NVIDIA:mainfrom
mhaseeb123:fea/two-step-materialize

Conversation

@mhaseeb123

@mhaseeb123 mhaseeb123 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Description

Contributes to #22583

This PR adds two-step column materialization APIs for the multifile hybrid scan reader

Checklist

mhaseeb123 and others added 30 commits May 21, 2026 01:15
Co-authored-by: Yunsong Wang <12716979+PointKernel@users.noreply.github.com>
Co-authored-by: Yunsong Wang <12716979+PointKernel@users.noreply.github.com>
@mhaseeb123
mhaseeb123 requested a review from Matt711 June 10, 2026 22:08
@mhaseeb123
mhaseeb123 marked this pull request as ready for review June 13, 2026 00:51

@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/src/io/parquet/experimental/hybrid_scan_multifile.cpp`:
- Around line 174-212: The construct_row_group_passes function is a public API
wrapper that is missing the CUDF_FUNC_RANGE() macro required by coding
guidelines. Add CUDF_FUNC_RANGE() as the first statement in the function body
before the line calculating total_row_groups using std::accumulate, ensuring it
comes before any other function logic to properly instrument the public API
delegation to _impl.
🪄 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: a18d206e-75d7-4707-807f-53f1e46f8a52

📥 Commits

Reviewing files that changed from the base of the PR and between 1949b00 and 48c2eca.

📒 Files selected for processing (2)
  • cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
  • cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

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/src/io/parquet/experimental/hybrid_scan_multifile.cpp`:
- Around line 174-212: The construct_row_group_passes function is a public API
wrapper that is missing the CUDF_FUNC_RANGE() macro required by coding
guidelines. Add CUDF_FUNC_RANGE() as the first statement in the function body
before the line calculating total_row_groups using std::accumulate, ensuring it
comes before any other function logic to properly instrument the public API
delegation to _impl.
🪄 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: a18d206e-75d7-4707-807f-53f1e46f8a52

📥 Commits

Reviewing files that changed from the base of the PR and between 1949b00 and 48c2eca.

📒 Files selected for processing (2)
  • cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
  • cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp
🛑 Comments failed to post (1)
cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp (1)

174-212: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add CUDF_FUNC_RANGE() to the new public API wrapper.

construct_row_group_passes is a public API in cpp/src that delegates to _impl, but it skips CUDF_FUNC_RANGE(), unlike adjacent public wrappers.

As per coding guidelines, public functions in cpp/src/**/*.{cu,cpp} should include CUDF_FUNC_RANGE() before delegating to detail implementations.

Suggested patch
 std::vector<std::vector<std::vector<size_type>>> hybrid_scan_multifile::construct_row_group_passes(
   cudf::host_span<std::vector<size_type> const> row_group_indices,
   std::size_t pass_read_limit) const
 {
+  CUDF_FUNC_RANGE();
+
   auto const total_row_groups =
     std::accumulate(row_group_indices.begin(),
                     row_group_indices.end(),
                     std::size_t{0},
                     [](auto sum, auto const& rgs) { return sum + rgs.size(); });
🤖 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/src/io/parquet/experimental/hybrid_scan_multifile.cpp` around lines 174 -
212, The construct_row_group_passes function is a public API wrapper that is
missing the CUDF_FUNC_RANGE() macro required by coding guidelines. Add
CUDF_FUNC_RANGE() as the first statement in the function body before the line
calculating total_row_groups using std::accumulate, ensuring it comes before any
other function logic to properly instrument the public API delegation to _impl.

Source: Coding guidelines

@mhaseeb123
mhaseeb123 requested a review from qbacpey June 25, 2026 01:04
Comment thread cpp/tests/io/experimental/hybrid_scan_multifile_composer.cpp Outdated
@mhaseeb123
mhaseeb123 requested a review from qbacpey June 26, 2026 23:09
@mhaseeb123

Copy link
Copy Markdown
Contributor Author

/merge

Comment thread cpp/tests/CMakeLists.txt
ConfigureTest(
HYBRID_SCAN_TEST
io/experimental/hybrid_scan_composer.cpp
io/experimental/hybrid_scan_multifile_composer.cpp

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.

Please sort these files alphabetically.

@rapids-bot
rapids-bot Bot merged commit 05aeeb7 into NVIDIA:main Jun 29, 2026
136 checks passed
@mhaseeb123
mhaseeb123 deleted the fea/two-step-materialize branch August 4, 2026 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 - In Progress Currently a work in progress CMake CMake build issue cuIO cuIO issue 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.

4 participants