Remove multi-file secondary-filter API and add bloom-filter pruning to hybrid scan - #22861
Merged
Conversation
- Renamed `secondary_filters_byte_ranges` to `bloom_filters_byte_ranges` for clarity and updated its return type to a single vector of byte ranges. - Introduced a new method `filter_row_groups_with_bloom_filters` to filter row groups using bloom filters. - Updated implementation in `hybrid_scan_impl` and `hybrid_scan_multifile` to support the new API. - Added tests for the new bloom filter functionality to ensure correctness. This change improves the usability and clarity of the hybrid scan multifile API regarding bloom filter operations.
|
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
commented
Jun 11, 2026
qbacpey
changed the title
secondary_filters_byte_ranges + add bloom pruning for multifile hybrid scansecondary_filters_byte_ranges + add bloom pruning for hybrid scan
qbacpey
added
3 - Ready for Review
qbacpey
marked this pull request as ready for review
June 11, 2026 21:19
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces the combined secondary-filter range API with dedicated bloom-filter range and row-group filtering APIs. Metadata now includes source-index mappings. C++ and Python tests cover multifile filtering, absent lengths, and mixed bloom-filter availability. ChangesBloom-filter API refactoring
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Around line 155-183: Add a deprecated compatibility overload for
secondary_filters_byte_ranges in the hybrid_scan_multifile public header that
forwards to the new implementations; annotate the overload with the
[[deprecated]] attribute and include a `@deprecated` doxygen tag explaining the
replacement and migration path, ensure the deprecated function signature matches
the previous public API and internally calls the appropriate new methods
(bloom_filters_byte_ranges and/or filter_row_groups_with_bloom_filters) so
existing users continue to link until they migrate, and update the PR labels per
deprecation/breaking policy.
🪄 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: 9d712f50-a460-45ad-a0c1-986440a86afe
📒 Files selected for processing (5)
cpp/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/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
mhaseeb123
reviewed
Jun 13, 2026
mhaseeb123
requested changes
Jun 13, 2026
mhaseeb123
reviewed
Jun 13, 2026
- Updated `bloom_filters_byte_ranges` to return a pair of byte ranges and a source-index map for better tracking of data origins. - Modified `filter_row_groups_with_bloom_filters` to accept a vector of device spans for bloom filter data, allowing for more flexible input. - Adjusted related methods and implementations in `hybrid_scan_helpers` and `hybrid_scan_impl` to accommodate the new return types and parameters. - Added tests to validate the new functionality and ensure correctness in handling bloom filters across multiple sources. These changes improve the API's usability and performance when working with bloom filters in hybrid scan operations.
qbacpey
commented
Jun 14, 2026
qbacpey
commented
Jun 14, 2026
| auto const bloom_filtered = reader->filter_row_groups_with_bloom_filters( | ||
| bloom_data_per_source, input_row_group_indices, options, stream); | ||
|
|
||
| // Shouldn't filter out any RG, since the queried value is present in every source. |
Contributor
Author
There was a problem hiding this comment.
Bloom filter should not filter out any data, since "Did not like the color" already in the RG
- Revised comment in `FilterRowGroupsWithBloomFiltersRealData` test to enhance clarity regarding the use of the embedded bloom-filter fixture. - This change improves the readability of the test code, making it easier to understand the context of the test setup.
qbacpey
commented
Jun 14, 2026
Comment on lines
+544
to
+562
| auto const [bloom_byte_ranges, bloom_source_map] = | ||
| reader->bloom_filters_byte_ranges(input_row_group_indices, options); | ||
| ASSERT_EQ(bloom_byte_ranges.size(), static_cast<size_t>(num_sources)); | ||
| ASSERT_EQ(bloom_byte_ranges.size(), bloom_source_map.size()); | ||
| std::vector<cudf::size_type> expected_source_map(num_sources); | ||
| std::iota(expected_source_map.begin(), expected_source_map.end(), 0); | ||
| EXPECT_EQ(bloom_source_map, expected_source_map); | ||
| EXPECT_TRUE(std::none_of(bloom_byte_ranges.begin(), bloom_byte_ranges.end(), [](auto const& r) { | ||
| return r.is_empty(); | ||
| })); | ||
|
|
||
| std::vector<std::vector<byte_range_info>> ranges_per_source(num_sources); | ||
| for (size_t i = 0; i < bloom_byte_ranges.size(); ++i) { | ||
| ASSERT_LT(bloom_source_map[i], num_sources); | ||
| ranges_per_source[bloom_source_map[i]].push_back(bloom_byte_ranges[i]); | ||
| } | ||
| auto [bloom_buffers, bloom_data_per_source, bloom_tasks] = | ||
| cudf::io::parquet::fetch_byte_ranges_to_device_async( | ||
| datasource_refs, ranges_per_source, stream, aligned_mr); |
Contributor
Author
There was a problem hiding this comment.
Alternative would be return a nested vector<vector<byte_range_info>> so that we can drop straight into the fetch & filter APIs with no regroup. In this case a fully-pruned source is just an empty inner vector, but that materializes an empty vector per pruned source and diverges from the flat single-file shape.
qbacpey
commented
Jun 15, 2026
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.hpp (1)
109-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroken
@copydoctarget forsecondary_filters_byte_ranges.At Line 109,
@copydocpoints tocudf::io::experimental::hybrid_scan_multifile::secondary_filters_byte_ranges, but that multifile symbol is no longer present after this API split. This leaves an unresolved doc reference and can fail Doxygen lint; switch to a valid symbol or replace with explicit Doxygen text.As per coding guidelines, C++/CUDA code must include proper Doxygen documentation.
🤖 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.hpp` around lines 109 - 113, The `@copydoc` directive on the secondary_filters_byte_ranges method declaration references a non-existent symbol (cudf::io::experimental::hybrid_scan_multifile::secondary_filters_byte_ranges) that was removed during the API split. Replace the broken `@copydoc` reference with explicit Doxygen documentation that describes what the secondary_filters_byte_ranges method does, including details about its parameters (row_group_indices and options) and its return value (the pair of byte range vectors), rather than attempting to copy documentation from a symbol that no longer exists.Source: Coding guidelines
🤖 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/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp`:
- Around line 612-619: The test_all_true_row_mask lambda function in the test
only validates metadata of the row mask (type, size, null_count) but does not
actually verify that the mask values are all true. Add a value assertion after
the existing checks to confirm that all elements in the row_mask are actually
true. This can be done by checking that the true_count equals the size of the
mask, or by using an algorithm like all_of to verify every element is true.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp`:
- Around line 109-113: The `@copydoc` directive on the
secondary_filters_byte_ranges method declaration references a non-existent
symbol
(cudf::io::experimental::hybrid_scan_multifile::secondary_filters_byte_ranges)
that was removed during the API split. Replace the broken `@copydoc` reference
with explicit Doxygen documentation that describes what the
secondary_filters_byte_ranges method does, including details about its
parameters (row_group_indices and options) and its return value (the pair of
byte range vectors), rather than attempting to copy documentation from a symbol
that no longer exists.
🪄 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: 2ea1b856-0072-4901-8fa1-2abc2d6fc49b
📒 Files selected for processing (4)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.hppcpp/src/io/parquet/experimental/hybrid_scan_multifile.cppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
🚧 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.hpp (1)
109-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroken
@copydoctarget forsecondary_filters_byte_ranges.At Line 109,
@copydocpoints tocudf::io::experimental::hybrid_scan_multifile::secondary_filters_byte_ranges, but that multifile symbol is no longer present after this API split. This leaves an unresolved doc reference and can fail Doxygen lint; switch to a valid symbol or replace with explicit Doxygen text.As per coding guidelines, C++/CUDA code must include proper Doxygen documentation.
🤖 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.hpp` around lines 109 - 113, The `@copydoc` directive on the secondary_filters_byte_ranges method declaration references a non-existent symbol (cudf::io::experimental::hybrid_scan_multifile::secondary_filters_byte_ranges) that was removed during the API split. Replace the broken `@copydoc` reference with explicit Doxygen documentation that describes what the secondary_filters_byte_ranges method does, including details about its parameters (row_group_indices and options) and its return value (the pair of byte range vectors), rather than attempting to copy documentation from a symbol that no longer exists.Source: Coding guidelines
🤖 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/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp`:
- Around line 612-619: The test_all_true_row_mask lambda function in the test
only validates metadata of the row mask (type, size, null_count) but does not
actually verify that the mask values are all true. Add a value assertion after
the existing checks to confirm that all elements in the row_mask are actually
true. This can be done by checking that the true_count equals the size of the
mask, or by using an algorithm like all_of to verify every element is true.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp`:
- Around line 109-113: The `@copydoc` directive on the
secondary_filters_byte_ranges method declaration references a non-existent
symbol
(cudf::io::experimental::hybrid_scan_multifile::secondary_filters_byte_ranges)
that was removed during the API split. Replace the broken `@copydoc` reference
with explicit Doxygen documentation that describes what the
secondary_filters_byte_ranges method does, including details about its
parameters (row_group_indices and options) and its return value (the pair of
byte range vectors), rather than attempting to copy documentation from a symbol
that no longer exists.
🪄 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: 2ea1b856-0072-4901-8fa1-2abc2d6fc49b
📒 Files selected for processing (4)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_impl.hppcpp/src/io/parquet/experimental/hybrid_scan_multifile.cppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
🚧 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/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp (1)
612-619:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
BuildAllTrueRowMasktest does not verify the mask is actually all true.At Lines 616-619, the test only checks metadata (
type,size,null_count). A mask filled withfalsevalues would still pass. Add a value assertion (all_of == trueor true-count == size).Suggested test assertion patch
auto const row_mask = reader->build_all_true_row_mask(row_group_indices, stream, mr); EXPECT_EQ(row_mask->type().id(), cudf::type_id::BOOL8); EXPECT_EQ(row_mask->size(), reader->total_rows_in_row_groups(row_group_indices)); EXPECT_EQ(row_mask->null_count(), 0); + auto const host_mask = host_row_mask_data<bool>(row_mask->view(), stream); + EXPECT_TRUE(std::all_of(host_mask.begin(), host_mask.end(), [](bool v) { return v; }));🤖 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/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp` around lines 612 - 619, The test_all_true_row_mask lambda function in the test only validates metadata of the row mask (type, size, null_count) but does not actually verify that the mask values are all true. Add a value assertion after the existing checks to confirm that all elements in the row_mask are actually true. This can be done by checking that the true_count equals the size of the mask, or by using an algorithm like all_of to verify every element is true.
mhaseeb123
requested changes
Jun 15, 2026
mhaseeb123
left a comment
mhaseeb123
left a comment
Contributor
There was a problem hiding this comment.
Flush comments so far
qbacpey
added
4 - Needs Review
mhaseeb123
approved these changes
Aug 17, 2026
qbacpey
added
5 - Ready to Merge
Contributor
Author
|
/merge |
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
Closes #23393
Contributes to #22583
This PR:
hybrid_scan_multifile::secondary_filters_byte_ranges.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