Hybrid scan reports correct number of input RGs - #23052
Merged
rapids-bot[bot] merged 5 commits intoJul 7, 2026
Merged
Conversation
|
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. |
mhaseeb123
added
bug
mhaseeb123
commented
Jun 30, 2026
| // Other valid types in the future would be UUID (still treated as string) and FLOAT16 (which | ||
| // for now would also be treated as a string). | ||
|
|
||
| /** |
Contributor
Author
There was a problem hiding this comment.
Simply reformatted
| return logical_type->type != LogicalType::DECIMAL; | ||
| } | ||
|
|
||
| /** |
| return output_dtypes; | ||
| } | ||
|
|
||
| /** |
| std::vector<std::vector<size_type>>(row_group_indices.size(), std::vector<size_type>{}); | ||
| prepare_data(read_mode::READ_ALL, empty_row_groups, {}, {}); | ||
| // Set correct number of input row groups to the output metadata | ||
| _file_itm_data.num_input_row_groups = count_row_groups(row_group_indices); |
Contributor
Author
There was a problem hiding this comment.
Make sure to write the correct number of input row groups as we will be materializing no row groups in this early exit
| auto const empty_row_groups = | ||
| std::vector<std::vector<size_type>>(row_group_indices.size(), std::vector<size_type>{}); | ||
| prepare_data(read_mode::CHUNKED_READ, empty_row_groups, {}, {}); | ||
| // Set correct number of input row groups to the output metadata |
| std::invalid_argument); | ||
| } | ||
|
|
||
| TEST_F(HybridScanTest, AllRowsPrunedReportsInputRowGroups) |
Contributor
Author
There was a problem hiding this comment.
Test:
- Small 10 row parquet file
- Set row mask to all false and read in two-steps (filter and payload)
- Should get empty table chunk out but the metadata should report 1 input row group
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds a ChangesHybrid Scan Pruned Row-Group Metadata Fix
Estimated code review effort: 2 (Simple) | ~12 minutes 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
🧹 Nitpick comments (1)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
539-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect fix, but consider consolidating the duplicated early-pruned logic.
The
num_input_row_groupsassignment correctly uses the originalrow_group_indices(not the zeroed-outempty_row_groups) and is applied afterprepare_dataso it isn't overwritten — this addresses the prior review feedback at the old lines 583/713 correctly.That said, this exact 3-statement pattern (build
empty_row_groups, callprepare_data, setnum_input_row_groups) is now duplicated across 4 call sites. This duplication is precisely why a prior PR (#23013) fixed only some of these spots and this followup PR had to patch the rest. Extracting a small private helper would prevent a future partial fix.♻️ Proposed helper to consolidate the early-pruned branches
+void hybrid_scan_reader_impl::prepare_for_all_rows_pruned( + read_mode mode, std::span<std::vector<size_type> const> row_group_indices) +{ + auto const empty_row_groups = + std::vector<std::vector<size_type>>(row_group_indices.size(), std::vector<size_type>{}); + prepare_data(mode, empty_row_groups, {}, {}); + // Set correct number of input row groups to the output metadata + _file_itm_data.num_input_row_groups = count_row_groups(row_group_indices); +}Then each call site collapses to, e.g.:
- auto const empty_row_groups = - std::vector<std::vector<size_type>>(row_group_indices.size(), std::vector<size_type>{}); - prepare_data(read_mode::READ_ALL, empty_row_groups, {}, {}); - // Set correct number of input row groups to the output metadata - _file_itm_data.num_input_row_groups = count_row_groups(row_group_indices); + prepare_for_all_rows_pruned(read_mode::READ_ALL, row_group_indices);Also applies to: 577-585, 648-656, 708-716
🤖 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 539 - 547, The early-pruned branch logic is duplicated in several places, including the code around read_chunk_internal and the other early-return call sites, where empty_row_groups is built, prepare_data is called, and _file_itm_data.num_input_row_groups is set. Extract that repeated 3-step sequence into a small private helper and have each pruned branch call it, so the num_input_row_groups assignment still happens after prepare_data and stays consistent across all sites.
🤖 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_test.cpp`:
- Around line 919-934: The metadata assertion in the hybrid scan test is
comparing against `row_groups.size()`, which counts sources rather than total
row groups and can miss regressions in `materialize_filter_columns`. Update the
`filter_result.metadata.num_input_row_groups` check to compare against the
actual row-group count derived from the nested `row_groups` structure, and use a
signed-safe comparison helper such as `std::cmp_equal` to avoid the
`size_type`/`size_t` mismatch. Make the same adjustment in the related payload
assertion block so both checks validate true row-group totals instead of source
count.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 539-547: The early-pruned branch logic is duplicated in several
places, including the code around read_chunk_internal and the other early-return
call sites, where empty_row_groups is built, prepare_data is called, and
_file_itm_data.num_input_row_groups is set. Extract that repeated 3-step
sequence into a small private helper and have each pruned branch call it, so the
num_input_row_groups assignment still happens after prepare_data and stays
consistent across all sites.
🪄 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: db150bf4-0ded-4462-bd3f-a630399fd993
📒 Files selected for processing (2)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cppcpp/tests/io/experimental/hybrid_scan_test.cpp
Matt711
approved these changes
Jul 1, 2026
mhaseeb123
added
5 - Ready to Merge
wence-
approved these changes
Jul 3, 2026
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
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
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
Checklist