Relax parquet page index requirements in hybrid scan - #23386
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. |
| // Compute the page index (column index and/or offset index) byte range | ||
| [[nodiscard]] byte_range_info page_index_byte_range(FileMetaData const& file_metadata) | ||
| { | ||
| auto const& row_groups = file_metadata.row_groups; |
There was a problem hiding this comment.
Used to be: range of bytes between the start of column index of the very first column chunk AND the end of offset index of the very last column chunk.
Now: byte range of only column or offset index if one is present, otherwise the same range as before.
There was a problem hiding this comment.
Out of curiosity, are there Parquet writers that only write the offset or column index and not both?
There was a problem hiding this comment.
Yes and we have seen datasets with only offset index
|
|
||
| if (max_offset <= min_offset) { return {}; } | ||
| return {min_offset, max_offset - min_offset}; | ||
| return page_index_byte_range(file_metadata); |
There was a problem hiding this comment.
Use the helper above instead of in place computing the full byte range
There was a problem hiding this comment.
question: The helper also bounds min_offset > 0 whereas this replaced code did not. Is that an independent bug fix?
There was a problem hiding this comment.
Not a bug fix (negative min_offset would just be malicious), just a small defensive check. libcudf doesn't defend against malicious footers (like we don't validate inputs) in general otherwise.
| }); | ||
| } | ||
|
|
||
| std::unique_ptr<cudf::column> aggregate_reader_metadata::build_all_true_row_mask( |
There was a problem hiding this comment.
Nothing new here, this used to be inlined in hybrid_scan_impl.cpp. Moved here so it can act as fallback for its twin build_row_mask_with_page_index_stats defined in page_index_filter.cu
| colchunk_offset = find_colchunk_iter_offset(row_group, mapped_schema_idx); | ||
| } | ||
| auto& colchunk_offset = colchunk_offsets[col]; | ||
| CUDF_EXPECTS(parquet::detail::find_colchunk_iter_offset( |
There was a problem hiding this comment.
Use the helper instead of inline
|
|
||
| if (has_page_index and not col_meta.encoding_stats.has_value()) { | ||
| // Make sure that all column chunk pages are dictionary encoded | ||
| auto const only_dict_encoded_pages = [&]() { |
There was a problem hiding this comment.
No need to check the presence of whole page index here.
| auto const& offset_index = colchunk_iter->offset_index.value(); | ||
| auto const row_group_num_pages = offset_index.page_locations.size(); | ||
|
|
||
| col_chunk_page_offsets.push_back(col_chunk_page_offsets.back() + row_group_num_pages); | ||
|
|
||
| // For all pages in this column chunk, update page row offsets. | ||
| std::for_each( | ||
| cuda::counting_iterator<std::size_t>{0}, | ||
| cuda::counting_iterator{row_group_num_pages}, | ||
| [&](auto const page_idx) { | ||
| int64_t const first_row_idx = offset_index.page_locations[page_idx].first_row_index; | ||
| // For the last page, this is simply the total number of rows in the column chunk | ||
| int64_t const last_row_idx = | ||
| (page_idx < row_group_num_pages - 1) | ||
| ? offset_index.page_locations[page_idx + 1].first_row_index | ||
| : row_group.num_rows; | ||
|
|
||
| // Update the page row offsets. | ||
| page_row_offsets.push_back(page_row_offsets.back() + last_row_idx - first_row_idx); | ||
| }); |
There was a problem hiding this comment.
Whitespace changes here only.
| first_row_group.columns[cached_offset].schema_idx != mapped_schema_idx) { | ||
| colchunk_offset = find_colchunk_iter_offset(first_row_group, mapped_schema_idx); | ||
| } | ||
| CUDF_EXPECTS( |
There was a problem hiding this comment.
Use helper instead of inline
| }); | ||
| }); | ||
| }); | ||
| std::for_each( |
There was a problem hiding this comment.
Whitespace changes only
| }); | ||
| std::for_each( | ||
| cuda::counting_iterator<std::size_t>{0}, | ||
| cuda::counting_iterator{row_group_indices.size()}, |
There was a problem hiding this comment.
Whitespace changes
| }); | ||
| }); | ||
| }); | ||
| std::for_each(cuda::counting_iterator<std::size_t>{0}, |
There was a problem hiding this comment.
Whitespace changes unfortunately.
| @@ -31,7 +31,6 @@ using metadata_base = parquet::detail::metadata; | |||
| using io::detail::inline_column_buffer; | |||
| using parquet::detail::CompactProtocolReader; | |||
There was a problem hiding this comment.
Note to reviewers: Please use the "Hide whitespace" feature to make this breezy
mhaseeb123
added
feature request
|
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:
📝 WalkthroughWalkthroughHybrid Parquet scans now validate page-index byte ranges, detect index availability for selected columns, fall back to unpruned masks when metadata is missing, require offset indexes for page-mask computation, and refine dictionary-page range discovery. ChangesParquet hybrid scan index handling
Estimated code review effort: 4 (Complex) | ~60 minutes 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp (4)
277-283: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftValidate every embedded page-index range before parsing.
Checking only the total buffer size does not prove that each
column_index_offset/lengthandoffset_index_offset/lengthlies within the buffer.metadata::setup_page_index()performs pointer arithmetic from those metadata values, so malformed intermediate offsets can cause out-of-bounds reads. Validate each range before calling it.As per coding guidelines: prevent invalid memory access and validate inputs before use.
🤖 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 277 - 283, Before calling metadata::setup_page_index in the page-index loading flow, validate every column_index_offset/length and offset_index_offset/length range from file_metadata lies within the expected page-index buffer, including overflow-safe bounds checks. Retain the existing non-empty and total-size validation, and reject any invalid embedded range before setup_page_index performs pointer arithmetic.Source: Coding guidelines
78-96: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winSearch for the first and last index-bearing chunks, not fixed endpoints.
If the physical first column chunk has no index but a later chunk does,
min_offsetbecomes zero and the entire file is treated as having no page-index buffer. The same problem occurs when the physical last chunk lacks an index. Iterate over row groups/columns to find the first and last chunk with either requested index portion.🤖 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 78 - 96, The min_offset and max_offset calculations must search for index-bearing chunks rather than assuming row_groups.front().columns.front() and row_groups.back().columns.back() contain indexes. Update these lambdas to iterate row groups and columns, selecting the first and last chunk with either a column or offset index, while preserving the existing offset precedence and zero fallback when no index-bearing chunk exists.
120-126: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not discard the missing-column fallback.
The result is assigned into an optional, but the updated helper always returns a value or throws. Consequently,
has_colchunkis always true and missing column chunks abort the scan instead of producing the intended unfiltered fallback. Align this call site with the final lookup contract and preserve the fallback path.🤖 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 120 - 126, Update the colchunk lookup around find_colchunk_iter_offset so missing column chunks remain representable as an empty optional instead of aborting the scan. Align the call site with the helper’s value-or-throw contract by handling its failure before assigning the result, preserving the existing has_colchunk, column-index, offset-index, and unfiltered fallback behavior.
595-599: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAlign dictionary-page lookup with the finalized API contract.
This call still stores the result in an optional and unconditionally calls
.value(). If lookup failures are meant to support fallback, check fornullopt; otherwise remove the optional cache semantics consistently with the mandatory-return API.🤖 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 595 - 599, Update the dictionary-page lookup around find_colchunk_iter_offset to match its finalized return contract: if lookup failure supports fallback, test the returned optional before accessing it and implement the fallback; otherwise remove optional handling consistently and use the mandatory offset directly. Ensure col_chunk access never unconditionally calls value() on a potentially empty result.cpp/src/io/parquet/reader_impl_helpers.cpp (1)
60-76: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftResolve the
find_colchunk_iter_offsetAPI contract before merging.The implementation now returns mandatory
size_typeand throws when absent, but callers still model lookup as optional;get_column_metadataadditionally calls.value()on the non-optional result. This breaks compilation and removes the fallback behavior required by the PR.
cpp/src/io/parquet/reader_impl_helpers.cpp#L60-L76: either restore optional failure results or update all callers to the mandatory-return contract.cpp/src/io/parquet/reader_impl_helpers.hpp#L122-L125: keep the declaration aligned with the chosen semantics.cpp/src/io/parquet/reader_impl_helpers.cpp#L1237-L1239: remove.value()if the return remainssize_type.cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp#L120-L126: preserve missing-column fallback handling.cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp#L595-L599: handle lookup failure before dereferencing the optional cache.rg -n -C3 'find_colchunk_iter_offset' cpp/src/io/parquet🤖 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/reader_impl_helpers.cpp` around lines 60 - 76, Resolve the find_colchunk_iter_offset contract consistently across all affected sites: choose either optional failure results with existing fallback behavior or mandatory size_type results with exception handling, then align its declaration in cpp/src/io/parquet/reader_impl_helpers.hpp:122-125. For the mandatory contract, remove .value() in reader_impl_helpers.cpp:1237-1239; in hybrid_scan_helpers.cpp:120-126 preserve missing-column fallback handling and in hybrid_scan_helpers.cpp:595-599 handle lookup failure before dereferencing the optional cache.
🤖 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/reader_impl_helpers.cpp`:
- Around line 64-67: Update the cached offset validation before indexing
row_group.columns: in the condition guarding the cached return, require
cached_offset.value() >= 0 before checking it against row_group.columns.size().
Preserve the existing schema_idx match and return behavior for valid offsets.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp`:
- Around line 277-283: Before calling metadata::setup_page_index in the
page-index loading flow, validate every column_index_offset/length and
offset_index_offset/length range from file_metadata lies within the expected
page-index buffer, including overflow-safe bounds checks. Retain the existing
non-empty and total-size validation, and reject any invalid embedded range
before setup_page_index performs pointer arithmetic.
- Around line 78-96: The min_offset and max_offset calculations must search for
index-bearing chunks rather than assuming row_groups.front().columns.front() and
row_groups.back().columns.back() contain indexes. Update these lambdas to
iterate row groups and columns, selecting the first and last chunk with either a
column or offset index, while preserving the existing offset precedence and zero
fallback when no index-bearing chunk exists.
- Around line 120-126: Update the colchunk lookup around
find_colchunk_iter_offset so missing column chunks remain representable as an
empty optional instead of aborting the scan. Align the call site with the
helper’s value-or-throw contract by handling its failure before assigning the
result, preserving the existing has_colchunk, column-index, offset-index, and
unfiltered fallback behavior.
- Around line 595-599: Update the dictionary-page lookup around
find_colchunk_iter_offset to match its finalized return contract: if lookup
failure supports fallback, test the returned optional before accessing it and
implement the fallback; otherwise remove optional handling consistently and use
the mandatory offset directly. Ensure col_chunk access never unconditionally
calls value() on a potentially empty result.
In `@cpp/src/io/parquet/reader_impl_helpers.cpp`:
- Around line 60-76: Resolve the find_colchunk_iter_offset contract consistently
across all affected sites: choose either optional failure results with existing
fallback behavior or mandatory size_type results with exception handling, then
align its declaration in cpp/src/io/parquet/reader_impl_helpers.hpp:122-125. For
the mandatory contract, remove .value() in reader_impl_helpers.cpp:1237-1239; in
hybrid_scan_helpers.cpp:120-126 preserve missing-column fallback handling and in
hybrid_scan_helpers.cpp:595-599 handle lookup failure before dereferencing the
optional cache.
🪄 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: 2c6cb12a-f500-489b-a597-501f0cec0da6
📒 Files selected for processing (3)
cpp/src/io/parquet/experimental/hybrid_scan_helpers.cppcpp/src/io/parquet/reader_impl_helpers.cppcpp/src/io/parquet/reader_impl_helpers.hpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cpp/src/io/parquet/reader_impl_helpers.cpp (1)
64-68: 🩺 Stability & Availability | 🔴 CriticalReject negative cached offsets before indexing.
size_typeis signed, so a cached value of-1passesstd::cmp_less(..., row_group.columns.size())and is then used as a vector index, causing invalid memory access. Requirecached_offset.value() >= 0before the bounds check.Proposed fix
- if (cached_offset.has_value() and + if (cached_offset.has_value() and cached_offset.value() >= 0 and std::cmp_less(cached_offset.value(), row_group.columns.size()) and🤖 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/reader_impl_helpers.cpp` around lines 64 - 68, Update the cached_offset validation before indexing row_group.columns to require cached_offset.value() >= 0 in addition to the existing bounds and schema_idx checks. Preserve returning the cached offset only for non-negative, in-range values matching schema_idx.
🤖 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.
Duplicate comments:
In `@cpp/src/io/parquet/reader_impl_helpers.cpp`:
- Around line 64-68: Update the cached_offset validation before indexing
row_group.columns to require cached_offset.value() >= 0 in addition to the
existing bounds and schema_idx checks. Preserve returning the cached offset only
for non-negative, in-range values matching schema_idx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ebab86bc-dcf1-4410-b764-865e13d8f908
📒 Files selected for processing (1)
cpp/src/io/parquet/reader_impl_helpers.cpp
| if (not col_chunk.offset_index.has_value()) { return; } | ||
| // Skip this column chunk if it does not have an offset index. This is because the decode | ||
| // paths can use column-index-derived information only together with offset index data. | ||
| if (not col_chunk.offset_index.has_value()) { continue; } |
There was a problem hiding this comment.
Previously if a column didn't have offset index, did we stop page pruning for the whole file? And now with this change only the current column?
There was a problem hiding this comment.
Previously if a column didn't have offset index, did we stop page pruning for the whole file?
Yes and we still do if it's missing for any of the columns we care about, just not all columns. That said, all parquet writers in practice write the indexes either for all or no column - but spec allows partial - so this is just defensive. In such (so far non-existent) cases, having partial indexes is still helpful in reducing some preprocessing work.
kingcrimsontianyu
left a comment
kingcrimsontianyu
left a comment
There was a problem hiding this comment.
Looks good! Just had a high level question.
| assertThrows(CudfException.class, () -> | ||
| reader.materializeFilterColumns(survived, filterCols, true)); | ||
| try (HybridScanReader.FilterMaterializationResult filter_columns = | ||
| reader.materializeFilterColumns(survived, filterCols, true)) { |
There was a problem hiding this comment.
How would a user who sets usePageLevelPruning = true know that page leveling pruning did not occur?
There was a problem hiding this comment.
libcudf logs a warning in the fallback case.
There was a problem hiding this comment.
Actually, I think I agree that the build_row_mask_with_page_index_stats() should throw as it would make our lives a lot easier. Pushing a commit with some reverts now
| // Compute the page index (column index and/or offset index) byte range | ||
| [[nodiscard]] byte_range_info page_index_byte_range(FileMetaData const& file_metadata) | ||
| { | ||
| auto const& row_groups = file_metadata.row_groups; |
There was a problem hiding this comment.
Out of curiosity, are there Parquet writers that only write the offset or column index and not both?
paul-aiyedun
left a comment
paul-aiyedun
left a comment
There was a problem hiding this comment.
Java updates look good to me. @pmattione-nvidia FYI.
mhaseeb123
added
5 - Ready to Merge
|
/merge |
8 participants
Description
Contributes to #23519
Several advanced parquet features in Hybrid scan reader require only offset index portion of the page index (column index is only used in page stats based pruned and that too only if offset index is also available). This PR relaxes the
has_page_indexcomputations and checks to specific page index portions we need and want to operate on as well as offering fallbacks instead of throwing in certain APIs.TLDR; this is mostly a refactor and nothing new really is being added or any bugs being fixed.
Checklist