Add hybrid scan multifile chunked materializers - #23013
Merged
rapids-bot[bot] merged 59 commits intoJun 30, 2026
Merged
Conversation
Co-authored-by: Yunsong Wang <12716979+PointKernel@users.noreply.github.com>
Co-authored-by: Yunsong Wang <12716979+PointKernel@users.noreply.github.com>
…id-scan-multifile-row-mask
There was a problem hiding this comment.
Pull request overview
This PR extends the experimental Parquet hybrid-scan multifile reader with chunked materialization APIs (filter, payload, and all-columns) and updates the test harness to exercise chunked+multisource behavior.
Changes:
- Add public
hybrid_scan_multifilechunking APIs (setup_chunking_for_*,materialize_*_chunk,has_next_table_chunk) and wire them through to the implementation. - Refactor/centralize hybrid-scan test helpers into a shared
hybrid_scan_common.cppcompilation unit and update tests to cover chunked multifile materialization. - Update internal row-mask handling in the hybrid-scan implementation to support chunked workflows and empty-result paths.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp | Declares the new public chunked materialization APIs and documentation. |
| cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp | Implements the new multifile chunked API wrappers by forwarding to the impl. |
| cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp | Updates impl interface/docs and moves update_row_mask into the class. |
| cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp | Adjusts materialization/chunking behavior and output finalization for empty mask cases. |
| cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu | Adds per-source row-count scan and updates row-mask update implementation/null-mask handling. |
| cpp/tests/io/experimental/hybrid_scan_common.hpp | Refactors test helpers into declarations shared across test TUs. |
| cpp/tests/io/experimental/hybrid_scan_common.cpp | New shared test-helper implementation + explicit template instantiations. |
| cpp/tests/io/experimental/hybrid_scan_composer.cpp | Removes duplicated helper logic now centralized in hybrid_scan_common. |
| cpp/tests/io/experimental/hybrid_scan_multifile_composer.{hpp,cpp} | Adds chunked multifile test-composer helpers. |
| cpp/tests/io/experimental/hybrid_scan_multifile_test.cpp | Extends multifile tests to validate chunked materializers. |
| cpp/tests/io/experimental/hybrid_scan_{test,filters_test}.cpp | Updates includes due to test-helper refactor. |
| cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp | Updates includes due to test-helper refactor. |
| cpp/tests/io/experimental/hybrid_scan_multifile_common.hpp | Removes obsolete multifile-only helper header (moved into shared helpers). |
| cpp/tests/CMakeLists.txt | Adds hybrid_scan_common.cpp to the HYBRID_SCAN_TEST target sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
1068-1074: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: This BOOL8 check breaks the documented empty-row-mask fast path.
The public contract still says “All rows read if empty”, but the type assertion now runs first. That rejects valid empty-mask calls before
finalize_output()can return the unfiltered table.Suggested fix
- CUDF_EXPECTS(row_mask.type().id() == type_id::BOOL8, "Input row mask must be a boolean column"); - - // If the input row mask is empty, return the table as is. - if (row_mask.is_empty()) { + // If the input row mask is empty, return the table as is. + if (row_mask.is_empty()) { _output_chunk_produced = true; return {std::move(read_table), std::move(out_metadata)}; } + + CUDF_EXPECTS(row_mask.type().id() == type_id::BOOL8, "Input row mask must be a boolean column");🤖 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_impl.cpp` around lines 1068 - 1074, The BOOL8 validation in finalize_output() is currently blocking the documented empty-row-mask fast path. Update the logic in hybrid_scan_impl::finalize_output so the row_mask.is_empty() case is handled before the type check, allowing empty masks to return the unfiltered table as intended. Keep the CUDF_EXPECTS(type_id::BOOL8) guard for non-empty masks only, and preserve the existing _output_chunk_produced and return behavior.
🤖 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_impl.cpp`:
- Around line 505-506: The new non-empty row_mask guard in the filter-column
materialization path rejects valid zero-row row-group spans, so adjust the
checks in both filter-column entry points to allow an empty BOOL8 mask when the
selected row groups contain no rows. Update the logic around
build_all_true_row_mask() handling in the relevant materialization functions so
an empty input returns an empty filtered table instead of triggering
CUDF_EXPECTS.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 1068-1074: The BOOL8 validation in finalize_output() is currently
blocking the documented empty-row-mask fast path. Update the logic in
hybrid_scan_impl::finalize_output so the row_mask.is_empty() case is handled
before the type check, allowing empty masks to return the unfiltered table as
intended. Keep the CUDF_EXPECTS(type_id::BOOL8) guard for non-empty masks only,
and preserve the existing _output_chunk_produced and return behavior.
🪄 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: 48e59d68-8488-4642-a7c7-831467e8e85b
📒 Files selected for processing (3)
cpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.cpp
✅ Files skipped from review due to trivial changes (1)
- cpp/include/cudf/io/experimental/hybrid_scan.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
There was a problem hiding this comment.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
1068-1074: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: This BOOL8 check breaks the documented empty-row-mask fast path.
The public contract still says “All rows read if empty”, but the type assertion now runs first. That rejects valid empty-mask calls before
finalize_output()can return the unfiltered table.Suggested fix
- CUDF_EXPECTS(row_mask.type().id() == type_id::BOOL8, "Input row mask must be a boolean column"); - - // If the input row mask is empty, return the table as is. - if (row_mask.is_empty()) { + // If the input row mask is empty, return the table as is. + if (row_mask.is_empty()) { _output_chunk_produced = true; return {std::move(read_table), std::move(out_metadata)}; } + + CUDF_EXPECTS(row_mask.type().id() == type_id::BOOL8, "Input row mask must be a boolean column");🤖 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_impl.cpp` around lines 1068 - 1074, The BOOL8 validation in finalize_output() is currently blocking the documented empty-row-mask fast path. Update the logic in hybrid_scan_impl::finalize_output so the row_mask.is_empty() case is handled before the type check, allowing empty masks to return the unfiltered table as intended. Keep the CUDF_EXPECTS(type_id::BOOL8) guard for non-empty masks only, and preserve the existing _output_chunk_produced and return behavior.
🤖 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_impl.cpp`:
- Around line 505-506: The new non-empty row_mask guard in the filter-column
materialization path rejects valid zero-row row-group spans, so adjust the
checks in both filter-column entry points to allow an empty BOOL8 mask when the
selected row groups contain no rows. Update the logic around
build_all_true_row_mask() handling in the relevant materialization functions so
an empty input returns an empty filtered table instead of triggering
CUDF_EXPECTS.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 1068-1074: The BOOL8 validation in finalize_output() is currently
blocking the documented empty-row-mask fast path. Update the logic in
hybrid_scan_impl::finalize_output so the row_mask.is_empty() case is handled
before the type check, allowing empty masks to return the unfiltered table as
intended. Keep the CUDF_EXPECTS(type_id::BOOL8) guard for non-empty masks only,
and preserve the existing _output_chunk_produced and return behavior.
🪄 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: 48e59d68-8488-4642-a7c7-831467e8e85b
📒 Files selected for processing (3)
cpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.cpp
✅ Files skipped from review due to trivial changes (1)
- cpp/include/cudf/io/experimental/hybrid_scan.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
🛑 Comments failed to post (1)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
505-506: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HIGH: Empty filter masks are still valid for zero-row row-group spans.
build_all_true_row_mask()can legitimately produce a 0-row BOOL8 mask when the selected row groups contain no rows. These new guards now throw on that empty-input path instead of returning an empty filtered table.Suggested fix
- CUDF_EXPECTS(not row_mask.is_empty(), - "Row mask must be non-empty when materializing filter columns"); + CUDF_EXPECTS(row_mask.type().id() == type_id::BOOL8, "Row mask must be a boolean column"); + CUDF_EXPECTS(row_mask.size() == total_rows_in_row_groups(row_group_indices), + "Row mask size must match the selected row groups");Apply the same change in both filter-column entry points.
Also applies to: 591-592
🤖 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_impl.cpp` around lines 505 - 506, The new non-empty row_mask guard in the filter-column materialization path rejects valid zero-row row-group spans, so adjust the checks in both filter-column entry points to allow an empty BOOL8 mask when the selected row groups contain no rows. Update the logic around build_all_true_row_mask() handling in the relevant materialization functions so an empty input returns an empty filtered table instead of triggering CUDF_EXPECTS.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_impl.cpp`:
- Around line 513-518: The all-pruned fast path in hybrid_scan_impl.cpp is
zeroing out row-group metadata by passing empty row groups into prepare_data(),
which causes read_chunk_internal() to emit incorrect out_metadata. Update the
no-work branches in the hybrid scan path to preserve the original
row_group_indices structure when calling prepare_data(), so the downstream
metadata still reflects the caller’s submitted row groups; apply the same fix in
the other matching fast-path branches noted in hybrid_scan_impl.cpp.
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp`:
- Around line 511-519: The helper are_all_rows_pruned currently assumes a BOOL8
row_mask and reads it with row_mask.begin<bool>(), so add an explicit BOOL8
precondition in are_all_rows_pruned itself or enforce it at every caller before
the fast path invokes it. Use row_mask.type().id() against type_id::BOOL8 near
the are_all_rows_pruned logic in hybrid_scan_impl.hpp, and keep the existing
finalize_output validation as a fallback rather than the first type check.
🪄 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: 70326862-092c-4651-932a-f92c269453df
📒 Files selected for processing (5)
cpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.cppcpp/src/io/parquet/experimental/hybrid_scan_impl.hppcpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu
✅ Files skipped from review due to trivial changes (1)
- cpp/include/cudf/io/experimental/hybrid_scan.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
- cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu
mhaseeb123
commented
Jun 30, 2026
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION. | |||
| * SPDX-FileCopyrightText: Copyright (c) 2025-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
Contributor
Author
There was a problem hiding this comment.
Only doc changes
| * | ||
| * @param options Parquet reader options | ||
| * @return Vector of row group indices, one inner vector per source | ||
| * @return Vectors of row group indices, one per source |
Contributor
Author
There was a problem hiding this comment.
Only doc changes
Member
There was a problem hiding this comment.
nitpick: libcudf in general seems to communicate this as "A vector of vectors ..."
| rmm::device_async_resource_ref mr) | ||
| { | ||
| CUDF_EXPECTS(not row_group_indices.empty(), "Empty input row group indices encountered"); | ||
| CUDF_EXPECTS(std::cmp_equal(row_mask.size(), total_rows_in_row_groups(row_group_indices)), |
Contributor
Author
There was a problem hiding this comment.
Empty row groups are fine. you will just get an empty table output
| _expr_conv = build_converted_expression(options); | ||
|
|
||
| // Return early if all rows are pruned | ||
| if (are_all_rows_pruned(row_mask, stream)) { |
Contributor
Author
There was a problem hiding this comment.
row_mask can no longer be empty as it's ambiguous (throw all rows? keep all rows?).
| */ | ||
| [[nodiscard]] bool has_next_table_chunk(); | ||
|
|
||
| /** |
Contributor
Author
There was a problem hiding this comment.
Simply moved this to private section, also needs not be static
| return _impl->materialize_all_columns(row_group_indices, column_chunk_data, options, stream, mr); | ||
| } | ||
|
|
||
| void hybrid_scan_multifile::setup_chunking_for_filter_columns( |
Contributor
Author
There was a problem hiding this comment.
Simple pass through
| struct row_mask_update_fn { | ||
| bool is_nullable; | ||
| bool const* in_row_mask; | ||
| bitmask_type const* in_bitmask; |
Contributor
Author
There was a problem hiding this comment.
Just the relaxed device lambda from lhs converted to a functor
| __device__ bool operator()(cudf::size_type row_idx) const | ||
| { | ||
| if (is_nullable and not bit_is_set(bitmask, row_idx)) { return false; } | ||
| return not row_mask[row_idx]; |
Contributor
Author
There was a problem hiding this comment.
Same thing as above just inverted logic
| bool hybrid_scan_reader_impl::are_all_rows_pruned(cudf::column_view const& row_mask, | ||
| rmm::cuda_stream_view stream) const | ||
| { | ||
| return cudf::detail::all_of( |
Contributor
Author
There was a problem hiding this comment.
A row in input row mask is considered pruned if it is valid and set to false (null means we can't determine) so we let it pass for now and determine at the output.
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_impl.hpp`:
- Line 164: The new `@copydoc` references in hybrid_scan_impl.hpp are pointing to
the wrong namespace, so update each affected documentation tag to use
cudf::io::parquet::experimental instead of cudf::io::experimental. Make the
namespace correction consistently in the hybrid_scan_multifile-related copydocs
and the other changed `@copydoc` lines in this block so Doxygen can resolve the
public API symbols cleanly.
In `@cpp/tests/io/experimental/hybrid_scan_common.hpp`:
- Around line 46-56: `setup_page_indexes()` currently fetches page-index buffers
into temporary host spans, so the reader later sees freed memory when those
spans are used to build row masks. Update `setup_page_indexes()` in
`hybrid_scan_common.cpp` to accept a mutable owner (similar to
`multifile_inputs`) and persist the page-index buffers and their spans for the
lifetime of the reader, alongside the existing footer buffers/byte spans. Make
the new retained storage live in the shared hybrid scan input state so the spans
returned from `setup_page_indexes()` remain valid after the function exits.
In `@cpp/tests/io/experimental/hybrid_scan_multifile_composer.cpp`:
- Around line 123-158: The chunked paths in `filter_row_groups_with_stats()` can
end up with no materialized tables when all row groups are pruned, so the final
`concatenate_tables()` calls on `filter_tables` and `payload_tables` need an
empty-input fallback. Update the logic around
`setup_chunking_for_filter_columns`, `setup_chunking_for_payload_columns`, and
the `has_next_table_chunk()` loops to detect the zero-chunk case and return a
schema-aware empty table result instead of concatenating an empty vector in both
chunked branches.
In `@cpp/tests/io/experimental/hybrid_scan_multifile_composer.hpp`:
- Around line 73-79: The new declarations in hybrid_scan_multifile_composer.hpp
use std::string through std::vector<std::string>, but the header does not
include <string> directly and is relying on transitive includes. Add the missing
<string> include near the other standard headers so
chunked_hybrid_scan_multifile and any other declarations in this header remain
self-contained and robust against include-order changes.
🪄 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: 64ecd716-1a2a-42e9-9134-6ea7019bc6f0
📒 Files selected for processing (17)
cpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.cppcpp/src/io/parquet/experimental/hybrid_scan_impl.hppcpp/src/io/parquet/experimental/hybrid_scan_multifile.cppcpp/src/io/parquet/experimental/hybrid_scan_preprocess.cucpp/tests/CMakeLists.txtcpp/tests/io/experimental/hybrid_scan_common.cppcpp/tests/io/experimental/hybrid_scan_common.hppcpp/tests/io/experimental/hybrid_scan_composer.cppcpp/tests/io/experimental/hybrid_scan_filters_test.cppcpp/tests/io/experimental/hybrid_scan_multifile_common.hppcpp/tests/io/experimental/hybrid_scan_multifile_composer.cppcpp/tests/io/experimental/hybrid_scan_multifile_composer.hppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cppcpp/tests/io/experimental/hybrid_scan_multifile_test.cppcpp/tests/io/experimental/hybrid_scan_test.cpp
💤 Files with no reviewable changes (1)
- cpp/tests/io/experimental/hybrid_scan_multifile_common.hpp
qbacpey
approved these changes
Jun 30, 2026
This was referenced Jun 30, 2026
Matt711
approved these changes
Jun 30, 2026
| * | ||
| * @param options Parquet reader options | ||
| * @return Vector of row group indices, one inner vector per source | ||
| * @return Vectors of row group indices, one per source |
Member
There was a problem hiding this comment.
nitpick: libcudf in general seems to communicate this as "A vector of vectors ..."
mhaseeb123
added
5 - Ready to Merge
Contributor
Author
|
/merge |
TomAugspurger
pushed a commit
to TomAugspurger/pygdf
that referenced
this pull request
Jun 30, 2026
Contributes to NVIDIA#22583 This PR adds chunked materialization APIs for the multifile hybrid scan reader. Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) - Matthew Murray (https://github.com/Matt711) Approvers: - Bradley Dice (https://github.com/bdice) - Qi Chen (https://github.com/qbacpey) - Matthew Murray (https://github.com/Matt711) URL: NVIDIA#23013
3 tasks
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 7, 2026
Follow up from #23013 This PR fixes the hybrid scan reader to report the correct number of input row groups in output metadata when all rows are pruned via row mask (two-step read case) and an empty output table chunk is produced Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Matthew Murray (https://github.com/Matt711) - Lawrence Mitchell (https://github.com/wence-) URL: #23052
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Contributes to #22583
This PR adds chunked materialization APIs for the multifile hybrid scan reader.
Checklist