Add page-level I/O and materialization in Hybrid Scan - #23375
Add page-level I/O and materialization in Hybrid Scan#23375mhaseeb123 wants to merge 12 commits into
Conversation
Preserve page locations and variable-width offset state needed to safely reconstruct columns from a sparse subset of Parquet data pages.
Expose multifile page-range planning and consume the selected payload pages so hybrid scan avoids fetching pruned Parquet payload data.
|
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. |
Part of #23362 This PR includes bug fixes and supporting features needed to enable page pruning with page-level (sparse) I/O (for payload columns) in hybrid scan, that is upcoming in #23375 and includes end to end tests. Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Paul Mattione (https://github.com/pmattione-nvidia) - Vukasin Milovanovic (https://github.com/vuule) - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #23374
mhaseeb123
added
feature request
…sparse-page-io-hybrid
ce70f6f to
1cf8e93
Compare
mhaseeb123
changed the title
mhaseeb123
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds payload page-range APIs, sparse page-data chunking, page-level pass masks, sparse Parquet preprocessing, and multifile hybrid-scan coverage for pruning, dictionaries, missing offset indexes, and row-group ordering. ChangesSparse payload hybrid scan
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 |
|
|
||
| // Must be called as soon as we create the pass | ||
| set_pass_page_mask(data_page_mask); | ||
| setup_next_pass(column_chunk_data, data_page_mask); |
There was a problem hiding this comment.
set_pass_page_mask is now moved inside the setup_next_pass
|
|
||
| // Setup page information for the chunk (which we can access without decompressing) | ||
| setup_compressed_data(column_chunk_data); | ||
| if (_sparse_page_io) { |
There was a problem hiding this comment.
Use either the dense or sparse overloads of the setup_compressed_data and set_pass_page_mask APIs.
| * @param schema_indices Schema indices from the first source | ||
| * @return A pair indicating column-index and offset-index presence, respectively | ||
| */ | ||
| [[nodiscard]] std::pair<bool, bool> page_index_presence( |
There was a problem hiding this comment.
Simply moved from private to public scope
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
466-487: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
column_chunk_dataparameter doc for the sparse branch.The doc comments for
handle_chunking(line 470) andsetup_next_pass(line 483) still read "Device spans of buffers containing column chunk data." With_sparse_page_ioenabled,hybrid_scan_chunking.cunow passes page-level spans (one span per logical page) through this same parameter, not per-chunk column data. Update the doc to state that this parameter holds page-level data spans when sparse I/O is active, so future readers do not assume a fixed per-chunk buffer layout.🤖 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 466 - 487, Update the column_chunk_data parameter documentation in handle_chunking and setup_next_pass to describe page-level data spans when _sparse_page_io is enabled, while preserving the existing description for non-sparse usage.
🧹 Nitpick comments (3)
cpp/tests/io/experimental/hybrid_scan_multifile_composer.cpp (1)
170-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared setup and filter phase.
Lines 170-205 duplicate lines 103-140 of
chunked_hybrid_scan_multifilealmost exactly: options build, reader construction, page-index setup, row-group filtering, row-mask build, chunk limits, and the filter-column chunk loop. Only the payload phase differs. Extract a small helper that returns the reader, inputs, row groups, row mask, and filter tables. This keeps the two composers in sync when the filter path changes.🤖 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_composer.cpp` around lines 170 - 205, Extract the duplicated filter setup and materialization flow from the current composer and chunked_hybrid_scan_multifile into a shared helper, reusing the existing options/reader construction, page-index setup, row-group filtering, row-mask creation, chunk-limit configuration, and filter_tables loop. Have the helper return the reader, inputs, row groups, row mask, and filter tables needed by each payload phase, then update both callers to use it while preserving their distinct payload processing.cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
1418-1435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the outer
chunk_idxto avoid shadowing.Line 1420 declares
chunk_idxfor the accumulate pass. Line 1435 declares anotherchunk_idxin the mask-building loop. The two variables have different meanings in the same function. A future edit inside the inner loop could read the wrong one. Consider computing the offsets with an explicit scan and a distinct name.♻️ Proposed refactor
// Find the first logical page-data span for every column chunk. auto page_offsets = std::vector<std::size_t>(chunks.size()); - auto chunk_idx = std::size_t{0}; + auto offset_cursor = std::size_t{0}; auto const num_logical_pages = std::accumulate( chunks.begin(), chunks.end(), std::size_t{0}, [&](auto offset, auto const& chunk) { - page_offsets[chunk_idx++] = offset; + page_offsets[offset_cursor++] = offset; return offset + chunk.num_dict_pages + chunk.num_data_pages; });🤖 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 1418 - 1435, Rename the outer chunk index used by the page-offset accumulation in the surrounding function to a distinct name, such as a page-offset scan index, so it cannot be confused with the inner chunk_idx declared in the mask-building loop. Update only its declaration and use in the accumulate pass; preserve the inner loop’s chunk_idx and existing offset behavior.cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp (1)
197-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Doxygen documentation for the two new declarations.
payload_pages_byte_ranges(lines 197-202) and the newsetup_chunking_for_payload_columnsoverload (lines 269-277) have no@brief/@copydocblock. Every other declaration in this file, including the siblingsetup_chunking_for_payload_columnsoverload directly above it, documents parameters with a@copydoctag referencing the public API. Add matching@copydocblocks for both new declarations to keep the file consistent and to support Doxygen-based linting.As per coding guidelines, "Use doxygen as a documentation generator and linter for C++ and CUDA code" applies to this file.
Also applies to: 269-277
🤖 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 197 - 202, Add matching Doxygen `@copydoc` blocks for payload_pages_byte_ranges and the new setup_chunking_for_payload_columns overload, using the corresponding public API declarations as references. Place each block immediately above its declaration and document parameters consistently with the existing sibling overload.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/include/cudf/io/experimental/hybrid_scan_multifile.hpp`:
- Around line 270-271: Update the exception documentation for
hybrid_scan_reader_impl::payload_pages_byte_ranges to state cudf::logic_error,
matching the existing CUDF_EXPECTS behavior and
SparsePayloadPagesWithoutOffsetIndexes test; do not change the implementation
exception type.
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 97-110: Update dictionary_page_range to return std::nullopt before
accessing page_locations.front() when page_locations is empty, while preserving
the existing offset-range behavior for non-empty indexes. Add a concise Doxygen
comment describing the helper’s purpose, inputs, and optional return result,
consistent with get_output_types and count_row_groups.
In `@cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu`:
- Around line 196-207: Add the same positive-row-count precondition used by
setup_compressed_data to setup_sparse_compressed_data, asserting
_pass_itm_data->num_rows > 0 with the established error message before
calculating page counts. Keep the existing offset-index and page-span
validations unchanged.
---
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp`:
- Around line 466-487: Update the column_chunk_data parameter documentation in
handle_chunking and setup_next_pass to describe page-level data spans when
_sparse_page_io is enabled, while preserving the existing description for
non-sparse usage.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 1418-1435: Rename the outer chunk index used by the page-offset
accumulation in the surrounding function to a distinct name, such as a
page-offset scan index, so it cannot be confused with the inner chunk_idx
declared in the mask-building loop. Update only its declaration and use in the
accumulate pass; preserve the inner loop’s chunk_idx and existing offset
behavior.
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp`:
- Around line 197-202: Add matching Doxygen `@copydoc` blocks for
payload_pages_byte_ranges and the new setup_chunking_for_payload_columns
overload, using the corresponding public API declarations as references. Place
each block immediately above its declaration and document parameters
consistently with the existing sibling overload.
In `@cpp/tests/io/experimental/hybrid_scan_multifile_composer.cpp`:
- Around line 170-205: Extract the duplicated filter setup and materialization
flow from the current composer and chunked_hybrid_scan_multifile into a shared
helper, reusing the existing options/reader construction, page-index setup,
row-group filtering, row-mask creation, chunk-limit configuration, and
filter_tables loop. Have the helper return the reader, inputs, row groups, row
mask, and filter tables needed by each payload phase, then update both callers
to use it while preserving their distinct payload processing.
🪄 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: 697975d5-e5fa-4229-acef-d654727c9b5e
📒 Files selected for processing (17)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_chunking.cucpp/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/hybrid_scan_preprocess.cucpp/src/io/parquet/experimental/page_index_filter.cucpp/src/io/parquet/io_utils/parquet_io_utils.cppcpp/src/io/parquet/reader_impl.hppcpp/src/io/parquet/reader_impl_preprocess.cucpp/src/io/parquet/reader_impl_preprocess_utils.cucpp/src/io/parquet/reader_impl_preprocess_utils.cuhcpp/tests/io/experimental/hybrid_scan_multifile_composer.cppcpp/tests/io/experimental/hybrid_scan_multifile_composer.hppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cppcpp/tests/io/experimental/hybrid_scan_multifile_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
1419-1435: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the outer
chunk_idxand replace the side-effectingstd::accumulate.Line 1420 declares
chunk_idx, and line 1435 declares anotherchunk_idxthat shadows it inside the lambda capture. The shadowing compiles, but it makes the two counters hard to tell apart and can trigger-Wshadow. Thestd::accumulateat line 1421 also mutatespage_offsetsandchunk_idxinside the reduction operator. A plain loop states the intent directly.♻️ Proposed refactor
// Find the first logical page-data span for every column chunk. - auto page_offsets = std::vector<std::size_t>(chunks.size()); - auto chunk_idx = std::size_t{0}; - auto const num_logical_pages = std::accumulate( - chunks.begin(), chunks.end(), std::size_t{0}, [&](auto offset, auto const& chunk) { - page_offsets[chunk_idx++] = offset; - return offset + chunk.num_dict_pages + chunk.num_data_pages; - }); + auto page_offsets = std::vector<std::size_t>(chunks.size()); + auto num_logical_pages = std::size_t{0}; + for (std::size_t idx = 0; idx < chunks.size(); ++idx) { + page_offsets[idx] = num_logical_pages; + num_logical_pages += chunks[idx].num_dict_pages + chunks[idx].num_data_pages; + } CUDF_EXPECTS(page_data.size() == num_logical_pages, "Sparse page span count does not match the number of logical pages");🤖 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 1419 - 1435, Rename the outer page-offset indexing variable used by the logical-page count setup to a distinct name, leaving the inner chunk traversal variable in the column-order lambda unchanged. Replace the side-effecting std::accumulate that fills page_offsets with a straightforward loop over chunks that records each offset and advances the running logical-page count.
🤖 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_impl.cpp`:
- Around line 853-861: Guard the all-pages-pruned early return in the
surrounding scan flow so it only executes when page_data is non-empty. Preserve
the existing all_of check for non-empty page_data, allowing
setup_sparse_compressed_data to validate empty or mismatched input instead of
silently producing an empty result.
---
Nitpick comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 1419-1435: Rename the outer page-offset indexing variable used by
the logical-page count setup to a distinct name, leaving the inner chunk
traversal variable in the column-order lambda unchanged. Replace the
side-effecting std::accumulate that fills page_offsets with a straightforward
loop over chunks that records each offset and advances the running logical-page
count.
🪄 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: 6c7ae62e-2570-4878-8ba9-14c805d9060e
📒 Files selected for processing (17)
cpp/include/cudf/io/experimental/hybrid_scan_multifile.hppcpp/src/io/parquet/experimental/hybrid_scan_chunking.cucpp/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/hybrid_scan_preprocess.cucpp/src/io/parquet/experimental/page_index_filter.cucpp/src/io/parquet/io_utils/parquet_io_utils.cppcpp/src/io/parquet/reader_impl.hppcpp/src/io/parquet/reader_impl_preprocess.cucpp/src/io/parquet/reader_impl_preprocess_utils.cucpp/src/io/parquet/reader_impl_preprocess_utils.cuhcpp/tests/io/experimental/hybrid_scan_multifile_composer.cppcpp/tests/io/experimental/hybrid_scan_multifile_composer.hppcpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cppcpp/tests/io/experimental/hybrid_scan_multifile_test.cpp
🚧 Files skipped from review as they are similar to previous changes (15)
- cpp/src/io/parquet/experimental/page_index_filter.cu
- cpp/src/io/parquet/io_utils/parquet_io_utils.cpp
- cpp/tests/io/experimental/hybrid_scan_multifile_composer.hpp
- cpp/src/io/parquet/experimental/hybrid_scan_multifile.cpp
- cpp/src/io/parquet/reader_impl_preprocess.cu
- cpp/src/io/parquet/experimental/hybrid_scan_chunking.cu
- cpp/src/io/parquet/experimental/hybrid_scan_helpers.hpp
- cpp/src/io/parquet/reader_impl_preprocess_utils.cuh
- cpp/src/io/parquet/reader_impl.hpp
- cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
- cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu
- cpp/tests/io/experimental/hybrid_scan_multifile_test.cpp
- cpp/src/io/parquet/reader_impl_preprocess_utils.cu
- cpp/tests/io/experimental/hybrid_scan_multifile_composer.cpp
- cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp
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/src/io/parquet/experimental/hybrid_scan_impl.cpp (1)
538-560: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate offset-index topology before emitting sparse ranges.
page_index_presence(...).secondverifies index presence only. It does not verify thatfind_colchunk_iter_offset(...)returned a value or that each page location is valid.At Line [560],
colchunk_offset.value()throws when a selected column is missing. Invalid offsets, non-positive page sizes, empty page locations, or inconsistent page rows can also produce invalid ranges or a page-span count that does not match sparse preprocessing.Validate every selected chunk and its page locations before using them. Fall back to
get_input_column_chunk_byte_ranges()or raise a clear error when the topology is incomplete.As per coding guidelines: “Prevent invalid memory access” and “Validate inputs such as negative dimensions and null pointers.”
Also applies to: 594-617
🤖 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 538 - 560, Validate each selected column chunk and its page locations in the hybrid scan flow before dereferencing colchunk_offset or emitting sparse ranges. In the loops computing chunk_page_counts and handling the corresponding page ranges, reject missing offsets, empty locations, non-positive page sizes, invalid offsets, and inconsistent page-row metadata; fall back to get_input_column_chunk_byte_ranges() or raise a clear error when validation fails. Ensure the validated page-span count matches sparse preprocessing.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.
Outside diff comments:
In `@cpp/src/io/parquet/experimental/hybrid_scan_impl.cpp`:
- Around line 538-560: Validate each selected column chunk and its page
locations in the hybrid scan flow before dereferencing colchunk_offset or
emitting sparse ranges. In the loops computing chunk_page_counts and handling
the corresponding page ranges, reject missing offsets, empty locations,
non-positive page sizes, invalid offsets, and inconsistent page-row metadata;
fall back to get_input_column_chunk_byte_ranges() or raise a clear error when
validation fails. Ensure the validated page-span count matches sparse
preprocessing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d825116a-6343-42d7-9ec2-52efd19d1194
📒 Files selected for processing (6)
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_preprocess.cucpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cppcpp/tests/io/experimental/hybrid_scan_multifile_test.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/src/io/parquet/experimental/hybrid_scan_preprocess.cu
- cpp/tests/io/experimental/hybrid_scan_multifile_filters_test.cpp
- cpp/include/cudf/io/experimental/hybrid_scan_multifile.hpp
- cpp/tests/io/experimental/hybrid_scan_multifile_test.cpp
- cpp/src/io/parquet/experimental/hybrid_scan_impl.hpp
1 participant
Description
Part of #23362
This PR adds new hybrid scan APIs to support page-level (sparse) I/O for payload columns. This includes:
Checklist