Add multifile row group pruning with stats and byte ranges - #22715
Merged
rapids-bot[bot] merged 24 commits intoJun 4, 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>
github-actions
Bot
added
libcudf
mhaseeb123
marked this pull request as draft
May 29, 2026 19:26
|
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds new byte-range and stats-based row-group pruning APIs plus secondary filter byte-range retrieval to the public hybrid_scan_multifile wrapper, implements them as thin forwards to the internal reader, updates helper validation for row-group indices, and adds tests for byte-range error handling and stats-based pruning across multiple Parquet sources. ChangesPublic multi-file hybrid scan API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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_helpers.cpp (1)
379-382:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse aggregate schema mapping here, not source-0/raw local schema ids.
Lines 381 and 611 still build the equality-literal collector from
per_file_metadata[0].schema, and Lines 489-499 walkrow_group.columnsby rawschema_idx. That breaks the new multifile path when schemas differ across sources: a filter column that only exists in later files is typed against the wrong schema, and dictionary pruning can fail to find the matching local column chunk for that source. Please resolve these lookups through the aggregate schema tree / per-source schema maps instead of source 0 or raw local ids.Also applies to: 489-499, 609-612
🤖 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_helpers.cpp` around lines 379 - 382, The equality literal collector and the code walking row_group.columns use per_file_metadata[0].schema and raw schema_idx (e.g., equality_literals_collector{... per_file_metadata[0].schema}.get_literals() and loops over row_group.columns by schema_idx), which breaks multifile handling when source schemas differ; update these lookups to resolve types and column mappings through the aggregate schema tree and the per-source schema maps (use the aggregate mapping that produced output_dtypes/output_column_schemas to translate output column ids into the correct local column/chunk ids for each per_file_metadata entry) so equality_literals_collector is constructed with the aggregate-resolved schema mapping for that source and the row_group.column iteration uses the per-source local id mapped from the aggregate schema rather than raw schema_idx.
🧹 Nitpick comments (2)
cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp (1)
28-58: ⚡ Quick winAdd
CUDF_FUNC_RANGE()to all remaining public forwarding methods.
parquet_metadatas,page_index_byte_ranges,all_row_groups,total_rows_in_row_groups, andreset_column_selectioncurrently delegate withoutCUDF_FUNC_RANGE(), unlike the other public methods in this file.As per coding guidelines "Add CUDF_FUNC_RANGE() in public functions before delegating to detail:: functions".
🤖 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 28 - 58, The public forwarding methods parquet_metadatas, page_index_byte_ranges, setup_page_indexes, all_row_groups, total_rows_in_row_groups, and reset_column_selection lack the CUDF_FUNC_RANGE() macro; add CUDF_FUNC_RANGE() as the first statement in each of these functions (e.g., inside hybrid_scan_multifile::parquet_metadatas(), ::page_index_byte_ranges(), ::all_row_groups(), ::total_rows_in_row_groups(), and ::reset_column_selection()) before delegating to _impl->... so they match the file's other public functions and follow the coding guideline.cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp (1)
54-168: ⚡ Quick winComplete Doxygen tags for public API declarations.
Several public methods have
@brief/@param/@return, but the guideline requires full tag coverage (@throw, and@tparamwhere applicable) on public API functions.As per coding guidelines "Add Doxygen documentation tags (
@param,@return,@throw,@tparam,@brief) on all public API functions".🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp` around lines 54 - 168, Update the Doxygen comments for all public APIs in class hybrid_scan_multifile to include the full set of required tags: ensure each constructor (hybrid_scan_multifile), destructor (~hybrid_scan_multifile), and methods (parquet_metadatas, page_index_byte_ranges, setup_page_indexes, all_row_groups, total_rows_in_row_groups, reset_column_selection, filter_row_groups_with_byte_range, filter_row_groups_with_stats, secondary_filters_byte_ranges) have `@brief`, `@param` for each parameter, `@return` where applicable, and add `@throw` descriptions for exceptions the method may propagate (and `@tparam` if any template params are introduced later); keep descriptions concise and accurate and mention thrown exception types or conditions for each method.
🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Around line 114-115: total_rows_in_row_groups currently returns a 32-bit
size_type which can overflow; change the function signature to return a 64-bit
count (e.g., std::int64_t) and update its implementation to use a 64-bit
accumulator (std::int64_t) when summing row counts from the provided
cudf::host_span<std::vector<size_type> const> row_group_indices, and propagate
that 64-bit type to any related local variables and return sites (and update
callers if needed) so totals cannot be truncated; refer to the function
total_rows_in_row_groups and its implementation when making these changes.
In `@cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp`:
- Around line 203-206: The code returns opts_row_groups unchanged, allowing
invalid row-group indices to cause OOB later; update the branch that returns
options.get_row_groups() (opts_row_groups) to validate every supplied id against
per_file_metadata[src].row_groups.size() before returning: for each source index
src and each rg_index in opts_row_groups[src] ensure 0 <= rg_index <
per_file_metadata[src].row_groups.size(), and invoke CUDF_EXPECTS with a clear
message (e.g., "Requested row-group index out of range for data source") on
failure so callers like get_dictionary_page_bytes() cannot hit host-side OOB.
In `@cpp/src/io/parquet/experimental/page_index_filter.cu`:
- Around line 1001-1003: The precondition computing row_mask_offset + total_rows
can overflow; replace the single unsafe check with two safe checks: first ensure
row_mask_offset is <= row_mask.size(), then ensure total_rows <= row_mask.size()
- row_mask_offset (so the subtraction is safe). Update the CUDF_EXPECTS call(s)
around the row-mask validation in page_index_filter.cu (referencing
row_mask_offset, total_rows, row_mask.size(), and CUDF_EXPECTS) to perform these
two comparisons and throw the same std::invalid_argument message on failure.
In `@cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp`:
- Around line 264-274: The string predicate uses an ASCII-only literal
("000010000"); add a UTF-8 non-ASCII test case by creating another
cudf::string_scalar with a multibyte string (e.g., containing accented or CJK
characters), wrap it as cudf::ast::literal (similar to literal2), and build a
corresponding column reference/operation (like col_ref2 and filter2) so the test
exercises multibyte string pruning; update or append to the existing
literals/filters (literal_value2, literal2, col_ref2, filter2) to include this
non-ASCII variant and ensure it follows the same construction pattern and stream
handling as the ASCII case.
- Around line 82-303: Add focused TEST_F cases under
HybridScanMultifileFiltersTest that exercise the multifile path with null
values, sliced columns, boundary-size row groups, and multi-block parquet files:
for nulls create_parquet_with_stats with nullable columns and assert
filter_row_groups_with_stats prunes correctly; for sliced columns build buffers
then pass subspans from build_multifile_inputs and ensure
hybrid_scan_multifile->parquet_metadatas and page_index_byte_ranges still match
and setup_page_indexes succeeds; for boundary sizes generate files whose row
groups are exactly at page_size_for_ordered_tests and just over it and validate
all_row_groups, total_rows_in_row_groups and stats filtering; for multi-block
produce parquet files spanning multiple blocks and verify page_index_byte_ranges
are non-empty and filter_row_groups_with_stats/
filter_row_groups_with_byte_range behave as expected (including throwing when
appropriate). Use existing helpers (create_parquet_with_stats,
create_empty_parquet_with_stats, build_multifile_inputs) and methods
(cudf::io::parquet::experimental::hybrid_scan_multifile, page_index_byte_ranges,
setup_page_indexes, filter_row_groups_with_stats, all_row_groups,
total_rows_in_row_groups) and follow the pattern of the existing tests for
assertions and seeding.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp`:
- Around line 379-382: The equality literal collector and the code walking
row_group.columns use per_file_metadata[0].schema and raw schema_idx (e.g.,
equality_literals_collector{... per_file_metadata[0].schema}.get_literals() and
loops over row_group.columns by schema_idx), which breaks multifile handling
when source schemas differ; update these lookups to resolve types and column
mappings through the aggregate schema tree and the per-source schema maps (use
the aggregate mapping that produced output_dtypes/output_column_schemas to
translate output column ids into the correct local column/chunk ids for each
per_file_metadata entry) so equality_literals_collector is constructed with the
aggregate-resolved schema mapping for that source and the row_group.column
iteration uses the per-source local id mapped from the aggregate schema rather
than raw schema_idx.
---
Nitpick comments:
In `@cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Around line 54-168: Update the Doxygen comments for all public APIs in class
hybrid_scan_multifile to include the full set of required tags: ensure each
constructor (hybrid_scan_multifile), destructor (~hybrid_scan_multifile), and
methods (parquet_metadatas, page_index_byte_ranges, setup_page_indexes,
all_row_groups, total_rows_in_row_groups, reset_column_selection,
filter_row_groups_with_byte_range, filter_row_groups_with_stats,
secondary_filters_byte_ranges) have `@brief`, `@param` for each parameter, `@return`
where applicable, and add `@throw` descriptions for exceptions the method may
propagate (and `@tparam` if any template params are introduced later); keep
descriptions concise and accurate and mention thrown exception types or
conditions for each method.
In `@cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp`:
- Around line 28-58: The public forwarding methods parquet_metadatas,
page_index_byte_ranges, setup_page_indexes, all_row_groups,
total_rows_in_row_groups, and reset_column_selection lack the CUDF_FUNC_RANGE()
macro; add CUDF_FUNC_RANGE() as the first statement in each of these functions
(e.g., inside hybrid_scan_multifile::parquet_metadatas(),
::page_index_byte_ranges(), ::all_row_groups(), ::total_rows_in_row_groups(),
and ::reset_column_selection()) before delegating to _impl->... so they match
the file's other public functions and follow the coding guideline.
🪄 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: 4da93e7c-3073-4ebd-b381-17afa9f36198
📒 Files selected for processing (12)
cpp/CMakeLists.txtcpp/include/cudf/io/experimental/hybrid_scan.hppcpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan.cppcpp/src/io/parquet/experimental/hybrid_scan_helpers.cppcpp/src/io/parquet/experimental/hybrid_scan_helpers.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/page_index_filter.cucpp/tests/CMakeLists.txtcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
Contributor
|
@mhaseeb123 This is still marked as a draft. Is it ready for review? |
Contributor
Author
Hi @igorpeshansky, Not yet, this PR includes code from another one still in review (the line count of this PR would significantly reduce once we pull in from it). I will mark it ready and ping you once ready |
mhaseeb123
marked this pull request as ready for review
June 2, 2026 01:34
Contributor
Author
|
@igorpeshansky @bdice the PR is ready to review now. Should be pretty straightforward :) |
mhaseeb123
added
feature request
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp (1)
149-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd default value for stream parameter.
The
streamparameter lacks a default value. Public APIs in cuDF must provide defaults for stream and MR parameters.As per coding guidelines: "public APIs must have defaults" for stream parameters.
🔧 Proposed fix
[[nodiscard]] std::vector<std::vector<size_type>> filter_row_groups_with_stats( cudf::host_span<std::vector<size_type> const> row_group_indices, parquet_reader_options const& options, - rmm::cuda_stream_view stream) const; + rmm::cuda_stream_view stream = cudf::get_default_stream()) const;🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp` around lines 149 - 152, The public method filter_row_groups_with_stats currently requires an explicit rmm::cuda_stream_view stream argument; update its signature to provide a default stream value (e.g., rmm::cuda_stream_view{} or rmm::cuda_stream_default according to project convention) so callers need not pass a stream; modify the declaration of filter_row_groups_with_stats to set the default for the stream parameter and ensure any matching definitions/overloads in the corresponding .cpp remain consistent with the new default.
🧹 Nitpick comments (3)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp (3)
167-168: ⚡ Quick winConsider documenting input validation expectations.
Like the other filter methods, this signature lacks documentation about input preconditions. Consider adding a
@throwtag or notes for edge cases such as emptyrow_group_indicesor invalid row group indices.As per coding guidelines, public APIs should document input validation for invalid parameters.
🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp` around lines 167 - 168, Add input-validation documentation to the public API for secondary_filters_byte_ranges(cudf::host_span<std::vector<size_type> const> row_group_indices, parquet_reader_options const& options) const: describe preconditions (e.g., that row_group_indices must be non-empty and each size_type must be within valid row-group range for the file), specify behavior for empty or out-of-range indices, and include a `@throw` tag indicating which exception(s) will be raised for invalid parameters (or state that invalid inputs result in undefined behavior if intentional); reference the function name secondary_filters_byte_ranges and the parameter row_group_indices in the doc comment so callers know what is validated.
149-152: ⚡ Quick winConsider documenting input validation expectations.
Similar to
filter_row_groups_with_byte_range, this method lacks documentation about input preconditions or validation behavior. Consider adding a@throwtag or notes for edge cases like empty spans or invalid indices.As per coding guidelines, public APIs should document input validation for invalid parameters.
🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp` around lines 149 - 152, Add documentation for input validation and preconditions to the public API for filter_row_groups_with_stats: describe expected inputs (e.g., that row_group_indices (cudf::host_span<std::vector<size_type> const>) must be non-empty and contain valid row-group indices), what happens for empty spans, out-of-range or duplicate indices, and whether parquet_reader_options is validated; include explicit `@throw` tags for parameter validation failures (e.g., std::invalid_argument or out_of_range) and any behavior on empty input to match the style used by filter_row_groups_with_byte_range so callers know the contract for filter_row_groups_with_stats.
137-139: ⚡ Quick winConsider documenting input validation expectations.
The method signature lacks documentation about input preconditions or validation. Consider adding a
@throwtag or notes clarifying how the API handles edge cases such as emptyrow_group_indices, out-of-range indices, or invalid options.As per coding guidelines, public APIs in headers should document input validation for invalid parameters.
🤖 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp` around lines 137 - 139, Add documentation above the declaration of filter_row_groups_with_byte_range clarifying input validation and expected behavior: document `@param` row_group_indices expectations (e.g., may be empty), `@param` options expected valid state, return behavior for empty input (e.g., returns empty vector), and explicit `@throw` clauses for invalid inputs (e.g., throw std::out_of_range for out-of-range indices and std::invalid_argument for invalid options). Also add brief notes about whether the function performs bounds checking and ownership/immutability assumptions of the cudf::host_span to make the API contract clear.
🤖 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.
Outside diff comments:
In `@cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Around line 149-152: The public method filter_row_groups_with_stats currently
requires an explicit rmm::cuda_stream_view stream argument; update its signature
to provide a default stream value (e.g., rmm::cuda_stream_view{} or
rmm::cuda_stream_default according to project convention) so callers need not
pass a stream; modify the declaration of filter_row_groups_with_stats to set the
default for the stream parameter and ensure any matching definitions/overloads
in the corresponding .cpp remain consistent with the new default.
---
Nitpick comments:
In `@cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Around line 167-168: Add input-validation documentation to the public API for
secondary_filters_byte_ranges(cudf::host_span<std::vector<size_type> const>
row_group_indices, parquet_reader_options const& options) const: describe
preconditions (e.g., that row_group_indices must be non-empty and each size_type
must be within valid row-group range for the file), specify behavior for empty
or out-of-range indices, and include a `@throw` tag indicating which exception(s)
will be raised for invalid parameters (or state that invalid inputs result in
undefined behavior if intentional); reference the function name
secondary_filters_byte_ranges and the parameter row_group_indices in the doc
comment so callers know what is validated.
- Around line 149-152: Add documentation for input validation and preconditions
to the public API for filter_row_groups_with_stats: describe expected inputs
(e.g., that row_group_indices (cudf::host_span<std::vector<size_type> const>)
must be non-empty and contain valid row-group indices), what happens for empty
spans, out-of-range or duplicate indices, and whether parquet_reader_options is
validated; include explicit `@throw` tags for parameter validation failures (e.g.,
std::invalid_argument or out_of_range) and any behavior on empty input to match
the style used by filter_row_groups_with_byte_range so callers know the contract
for filter_row_groups_with_stats.
- Around line 137-139: Add documentation above the declaration of
filter_row_groups_with_byte_range clarifying input validation and expected
behavior: document `@param` row_group_indices expectations (e.g., may be empty),
`@param` options expected valid state, return behavior for empty input (e.g.,
returns empty vector), and explicit `@throw` clauses for invalid inputs (e.g.,
throw std::out_of_range for out-of-range indices and std::invalid_argument for
invalid options). Also add brief notes about whether the function performs
bounds checking and ownership/immutability assumptions of the cudf::host_span to
make the API contract clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b4661033-a02d-40dd-b6a1-e803155135cc
📒 Files selected for processing (1)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
igorpeshansky
left a comment
igorpeshansky
left a comment
Contributor
There was a problem hiding this comment.
/cc @pmattione-nvidia for awareness, as he was implementing something similar.
bdice
approved these changes
Jun 2, 2026
bdice
left a comment
bdice
left a comment
Contributor
There was a problem hiding this comment.
Really great ideas from @igorpeshansky -- I have no additional commentary.
mhaseeb123
added
4 - Needs Review
igorpeshansky
approved these changes
Jun 4, 2026
Contributor
Author
|
/merge |
This was referenced Jun 9, 2026
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
3 participants
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 hybrid scan multifile reader APIs to filter row groups using column chunk stats and byte ranges.
Checklist