Add multifile dictionary pruning support for hybrid scan parquet reader - #22866
Merged
Conversation
qbacpey
added
2 - In Progress
|
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. |
qbacpey
force-pushed
the
hybrid/dict-multi-t2
branch
from
June 12, 2026 15:00
87baf98 to
c99d05d
Compare
…d scan multifile reader Need to refact testing code reused.
…hybrid scan multifile filters test
…s and order. Update hybrid_scan_multifile_filters_test to validate mismatched schema handling for dictionary pruning. Remove unused write_mismatched_source function to streamline code.
qbacpey
force-pushed
the
hybrid/dict-multi-t2
branch
from
June 12, 2026 16:07
c99d05d to
cddcce3
Compare
qbacpey
commented
Jun 12, 2026
…a dedicated function for improved clarity and maintainability. Update error message formatting in page index filter utilities to utilize std::format for better readability.
Updated the `dictionary_pages_byte_ranges` method across multiple files to return a pair of vectors: one for the byte ranges of dictionary pages and another for their corresponding source indices. This change enhances the functionality and clarity of the data returned, facilitating better handling of dictionary page filtering in hybrid scan operations.
…d tests - Removed redundant comments in `hybrid_scan_helpers.cpp` for clarity. - Added validation checks in `create_parquet_with_stats` to ensure column names and order are consistent. - Updated comments in `hybrid_scan_multifile_filters_test.cpp` for better readability and understanding of filtering logic.
qbacpey
marked this pull request as ready for review
June 22, 2026 08:32
qbacpey
changed the title
|
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 dictionary-page byte-range and row-group filtering methods to ChangesDictionary-Page Pruning API and Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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_helpers.cpp`:
- Around line 489-499: In the nested std::for_each loops where row_group_indices
is iterated, add bounds validation in the inner lambda before accessing
per_file_metadata[src_index].row_groups[rg_index]. Specifically, after obtaining
src_index and rg_index, validate that src_index is within the bounds of
per_file_metadata and that rg_index is within the bounds of
per_file_metadata[src_index].row_groups before attempting to access the
row_group object. Add appropriate error handling or assertions to catch any
out-of-bounds conditions and prevent crashes from mismatched source counts or
invalid row-group indices.
🪄 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: ed6708c2-e0bc-4ae1-bab8-7d3fda38ce79
📒 Files selected for processing (9)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/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_utils.cucpp/tests/io/experimental/hybrid_scan_common.hppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
Comment on lines
489
to
+499
| // For all sources | ||
| std::for_each( | ||
| cuda::counting_iterator<std::size_t>{0}, | ||
| cuda::counting_iterator{row_group_indices.size()}, | ||
| [&](auto const src_index) { | ||
| // Get all row group indices in the data source | ||
| auto const& rg_indices = row_group_indices[src_index]; | ||
| std::optional<size_type> colchunk_iter_offset{}; | ||
| // For all row groups | ||
| std::for_each(rg_indices.cbegin(), rg_indices.cend(), [&](auto const rg_index) { | ||
| auto const& row_group = per_file_metadata[src_index].row_groups[rg_index]; | ||
| // For all column chunks | ||
| auto const& row_group = per_file_metadata[src_index].row_groups[rg_index]; | ||
| auto const num_col_chunks = static_cast<size_type>(row_group.columns.size()); |
There was a problem hiding this comment.
Validate row-group/source bounds before indexing metadata vectors.
At Line 498, per_file_metadata[src_index].row_groups[rg_index] is accessed without validating input shape/bounds first. If row_group_indices has a mismatched source count or an out-of-range row-group index, this can trigger out-of-bounds access and crash.
Suggested fix
std::pair<std::vector<byte_range_info>, std::vector<cudf::size_type>>
aggregate_reader_metadata::dictionary_pages_byte_ranges(
cudf::host_span<std::vector<cudf::size_type> const> row_group_indices,
host_span<data_type const> output_dtypes,
host_span<cudf::size_type const> output_column_schemas,
std::reference_wrapper<ast::expression const> filter)
{
+ CUDF_EXPECTS(row_group_indices.size() == per_file_metadata.size(),
+ "Row group indices must provide one vector per input source",
+ std::invalid_argument);
+ for (std::size_t src_index = 0; src_index < row_group_indices.size(); ++src_index) {
+ auto const num_row_groups = per_file_metadata[src_index].row_groups.size();
+ for (auto const rg_index : row_group_indices[src_index]) {
+ CUDF_EXPECTS(std::cmp_greater_equal(rg_index, 0) and
+ std::cmp_less(rg_index, num_row_groups),
+ "Encountered out-of-bounds row group index for data source",
+ std::invalid_argument);
+ }
+ }
+
// Collect (in)equality literals for each input table column
auto const literals = dictionary_literals_collector{filter.get(), output_dtypes}.get_literals();🤖 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 489 -
499, In the nested std::for_each loops where row_group_indices is iterated, add
bounds validation in the inner lambda before accessing
per_file_metadata[src_index].row_groups[rg_index]. Specifically, after obtaining
src_index and rg_index, validate that src_index is within the bounds of
per_file_metadata and that rg_index is within the bounds of
per_file_metadata[src_index].row_groups before attempting to access the
row_group object. Add appropriate error handling or assertions to catch any
out-of-bounds conditions and prevent crashes from mismatched source counts or
invalid row-group indices.
- Modified copyright statements in various source and header files to reflect the inclusion of "NVIDIA CORPORATION & AFFILIATES." - Updated files include `hybrid_scan_multifile.hpp`, `hybrid_scan_helpers.cpp`, `hybrid_scan_helpers.hpp`, `hybrid_scan_impl.cpp`, `hybrid_scan_impl.hpp`, `page_index_filter_utils.cu`, and test files related to hybrid scans.
igorpeshansky
left a comment
igorpeshansky
left a comment
Contributor
There was a problem hiding this comment.
One question about the API usage…
…e comment in `setup_multifile_page_index` for clarity
…ation This commit simplifies the handling of dictionary page byte ranges in the `filter_row_groups_with_dictionaries_impl` function. It removes unnecessary comments and consolidates the logic for fetching byte ranges, ensuring consistency in the use of row group indices across different reader types. This enhances code clarity and maintainability.
mhaseeb123
approved these changes
Jul 6, 2026
- Updated documentation in `reader_impl_helpers.hpp` to specify that the function finds the offset of the column chunk in the specified row group. - Simplified validation checks in `create_parquet_with_stats` by removing redundant conditions and directly enforcing that `column_order` must include all three test columns. - Replaced default test column names and order with inline definitions for clarity in `hybrid_scan_common.hpp`. These changes enhance code readability and maintainability while ensuring correct validation logic in the Parquet creation process.
…tions - Updated the `filter_row_groups_with_dictionaries` function to accept `parquet_reader_options` instead of `filter_expression`. - Adjusted related test cases in `hybrid_scan_filters_test.cpp` to build options using the new `parquet_reader_options` structure. These changes improve the flexibility and clarity of the filtering mechanism in the hybrid scan functionality.
mhaseeb123
approved these changes
Jul 13, 2026
vuule
approved these changes
Jul 15, 2026
vuule
left a comment
vuule
left a comment
Contributor
There was a problem hiding this comment.
looks good; few small comments.
| reader.secondary_filters_byte_ranges(row_group_indices, options).second; | ||
| CUDF_EXPECTS(dict_page_byte_ranges.size() > 0, "No dictionary page byte ranges found"); | ||
|
|
||
| auto [dict_page_buffers, dict_page_data, dict_page_tasks] = |
Contributor
There was a problem hiding this comment.
Suggested change
| auto [dict_page_buffers, dict_page_data, dict_page_tasks] = | |
| [[maybe_unused]] auto [_, dict_page_data, dict_page_tasks] = |
Contributor
There was a problem hiding this comment.
I am okay with letting them say dict_page_buffers as we need to keep them alive until the dict_page_data (spans into these buffers) are fully used by the reader
Contributor
There was a problem hiding this comment.
[[maybe_unused]] makes sense nonetheless
|
|
||
| auto const dict_page_ranges_per_source = | ||
| group_byte_ranges_by_source(dict_pages, inputs.datasources.size()); | ||
| auto [dict_page_buffers, dict_page_data_per_source, task] = |
Contributor
There was a problem hiding this comment.
Suggested change
| auto [dict_page_buffers, dict_page_data_per_source, task] = | |
| [[maybe_unused]] auto [_, dict_page_data_per_source, task] = |
Contributor
Author
|
/merge |
This was referenced Jul 21, 2026
This was referenced Jul 30, 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
7 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 add multifile dictionary pruning support for hybrid scanner
Note: The single-file
hybrid_scan_reader::secondary_filters_byte_rangesis intentionally retained and will be removed later in a coordinated PR alongside the Python/Java binding updates.Checklist