Prepend source index column in Parquet reader - #22879
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. |
| std::optional<int32_t> type_length; ///< Byte width of data (for fixed length data) | ||
| std::vector<column_name_info> children; ///< Child column names | ||
|
|
||
| /** |
There was a problem hiding this comment.
No need for explicit constructor or a == operator. Use designated initializers and default == operator works.
| return {filtered_row_group_indices}; | ||
| } | ||
|
|
||
| offset_column_references::offset_column_references( |
There was a problem hiding this comment.
Very simple transformer with lots of boilerplate. Simply offsets the column_references in the input tree by a specified number. That's it, it changes nothing else.
| out_metadata.num_rows_per_source = | ||
| std::vector<size_t>(_file_itm_data.num_rows_per_source.size(), 0); | ||
| } | ||
| // Empty dataframe case: Simply initialize to a list of zeros |
There was a problem hiding this comment.
Always write number of rows read from each source. Even if it's zero.
| // Move is okay here as we are reading in one go. | ||
| out_metadata.num_rows_per_source = std::move(_file_itm_data.num_rows_per_source); | ||
| } | ||
| // Compute the output number of rows per source |
There was a problem hiding this comment.
Same. Always write number of rows per source
| _file_itm_data._output_chunk_count++; | ||
|
|
||
| // Prepend the source index column if requested | ||
| if (_options.prepend_source_index_column) { |
There was a problem hiding this comment.
Build and prepend source index column using the rows read per source information which we now unconditionally write.
| if (_num_filter_only_columns > 0) { out_metadata.schema_info.resize(output_count); } | ||
|
|
||
| // Clear the number of rows per source as it is not valid after filtering | ||
| out_metadata.num_rows_per_source.clear(); |
There was a problem hiding this comment.
Clear rows per source information (which was previously guarded under the same condition) as applying a filter modifies it. Note that the prepended column also gets filter so it still remains valid.
| } | ||
|
|
||
| // Offset column references in `_expr_conv` by the number of prepended columns | ||
| auto const num_prepended_cols = static_cast<size_type>(_options.prepend_source_index_column); |
There was a problem hiding this comment.
We must now update the column references in our filter expression by the number of prepended columns so the filter expression works on correct data columns.
| return _file_itm_data._output_chunk_count == 0; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
No need for this anymore
| index_names.cend(), | ||
| std::back_inserter(selected_columns), | ||
| [](std::string const& name) { return column_name_info(name); }); | ||
| [](std::string const& name) { return column_name_info{.name = name}; }); |
There was a problem hiding this comment.
Use designated initializer
| auto col_data = | ||
| cudf::detail::make_zeroed_device_uvector_async<column_type>(num_rows, _stream, _mr); | ||
|
|
||
| // Multiple sources |
There was a problem hiding this comment.
Reused logic from multiple places. Start with a zeroed column, scatter (increasing) source indices to the column locations where they start contributing. Do an inclusive_scan with cuda::maximum to update those zeros to the last source index.
|
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:
📝 WalkthroughWalkthroughThis PR adds a source index column prepending feature to the Parquet reader, allowing users to track which input file each output row originated from. It introduces a new configuration option, implements GPU-accelerated column generation for single and multi-source cases, and integrates filter expression offset handling to maintain correctness during filtering. ChangesPrepend Source Index Column Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/reader_impl_preprocess.cu (1)
1130-1130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInteger overflow in
std::accumulatewith mismatched types.Using
0(int literal) as the initial value causesstd::accumulateto useintas the accumulator type. Sincenum_rows_per_sourcecontainsstd::size_tvalues, summing them into anintaccumulator causes signed integer overflow (undefined behavior) if the total exceedsINT_MAX.Suggested fix
- auto const num_rows = std::accumulate(num_rows_per_source.begin(), num_rows_per_source.end(), 0); + auto const num_rows = std::accumulate(num_rows_per_source.begin(), num_rows_per_source.end(), std::size_t{0});🤖 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_preprocess.cu` at line 1130, The std::accumulate call on num_rows_per_source uses an int literal 0 as the initial value, which causes the accumulator to be typed as int rather than matching the std::size_t type of the elements in num_rows_per_source. This creates a signed integer overflow vulnerability when summing values. Replace the initial value 0 with a std::size_t typed literal (such as 0UL or std::size_t(0)) to ensure the accumulator uses the same type as the elements being summed, preventing the integer overflow.
🤖 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/reader_impl_preprocess.cu`:
- Line 1130: The std::accumulate call on num_rows_per_source uses an int literal
0 as the initial value, which causes the accumulator to be typed as int rather
than matching the std::size_t type of the elements in num_rows_per_source. This
creates a signed integer overflow vulnerability when summing values. Replace the
initial value 0 with a std::size_t typed literal (such as 0UL or std::size_t(0))
to ensure the accumulator uses the same type as the elements being summed,
preventing the integer overflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f225f573-1bf6-4060-980b-ba3dd8973761
📒 Files selected for processing (2)
cpp/src/io/parquet/reader_impl_preprocess.cucpp/tests/io/parquet_chunked_reader_test.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/tests/io/parquet_chunked_reader_test.cu
| // Compute column chunk level page count offsets, and page level row counts and row offsets. | ||
| auto const [page_row_counts, page_row_offsets, col_chunk_page_offsets] = | ||
| compute_page_row_counts_and_offsets(per_file_metadata, row_group_indices, schema_idx, stream); | ||
| // Compute column chunk level page count offsets and page level row offsets. |
There was a problem hiding this comment.
This function now doesn't return page_row_counts (not needed anymore)
| auto [page_row_counts, page_row_offsets, col_chunk_page_offsets, min, max, is_null] = | ||
| // Compute page row offsets, column chunk page offsets, min, max and optional is_null stats | ||
| // host columns. | ||
| auto [page_row_offsets, col_chunk_page_offsets, min, max, is_null] = |
There was a problem hiding this comment.
Return type doesn't include page_row_counts anymore
| auto [page_row_counts, page_row_offsets, col_chunk_page_offsets, min, max, is_null] = | ||
| // Compute page row offsets, column chunk page offsets, min, max and optional is_null stats | ||
| // host columns. | ||
| auto [page_row_offsets, col_chunk_page_offsets, min, max, is_null] = |
There was a problem hiding this comment.
Return type doesn't include page_row_counts anymore
| stream, | ||
| cudf::get_current_device_resource_ref()); | ||
| // Construct a row indices mapping based on page row offsets. | ||
| auto const page_indices = compute_page_indices_async( |
There was a problem hiding this comment.
compute_page_indices_async doesn't need page_row_counts anymore
| page_indices.begin(), | ||
| cuda::maximum<cudf::size_type>()); | ||
| auto page_indices = rmm::device_uvector<cudf::size_type>(total_rows, stream, mr); | ||
| cudf::detail::label_segments( |
There was a problem hiding this comment.
Use detail::label_segments instead of manual computation
|
/merge |
Review feedback: revert the conflict-resolution judgment call from the NVIDIA#22879 backport that kept cudf::host_span on compute_has_page_index() and compute_page_row_offsets_and_colchunk_page_offsets(). Use std::span on these two signatures instead, matching upstream PR NVIDIA#22879/main exactly. Callers in page_index_filter.cu continue to pass cudf::host_span-typed members, which convert implicitly via host_span::operator std::span<T>(). compute_page_row_offsets() and compute_page_indices_async() are unrelated to this PR's diff and keep cudf::host_span as in the 26.06 base.
Contributes to NVIDIA#22849 This PR enables optionally prepending a source index column to the read table. Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Yunsong Wang (https://github.com/PointKernel) - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#22879
…snapshot] Backport of the effective diff of rapidsai/cudf PR NVIDIA#23077 ("Prepend row index column in Parquet reader"), which was still OPEN at the time this branch was created. This captures the PR's net change as a single squashed commit computed as the diff between its merge-base with main and its head: merge-base: 52d322c head: 2574b9d Applied on top of the 26.06.01 pin + NVIDIA#22773 + NVIDIA#22879 backport. NOTE: This is a PRE-MERGE snapshot; re-sync to the squashed merge commit once NVIDIA#23077 lands upstream. Unrelated main-only tests present as diff context (MismatchedSchema*) were intentionally excluded.
Review feedback: revert the conflict-resolution judgment call from the NVIDIA#22879 backport that kept cudf::host_span on compute_has_page_index() and compute_page_row_offsets_and_colchunk_page_offsets(). Use std::span on these two signatures instead, matching upstream PR NVIDIA#22879/main exactly. Callers in page_index_filter.cu continue to pass cudf::host_span-typed members, which convert implicitly via host_span::operator std::span<T>(). compute_page_row_offsets() and compute_page_indices_async() are unrelated to this PR's diff and keep cudf::host_span as in the 26.06 base.
Closes #22849. Follow up of #22879 + #23077 This PR enables prepending source and row index columns in hybrid scan. For two-step materialization, the extra index columns are prepended to filter columns only. Single-step materialization is identical to the main parquet reader Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Yunsong Wang (https://github.com/PointKernel) - Vyas Ramasubramani (https://github.com/vyasr) URL: #22878
Description
Contributes to #22849
This PR enables optionally prepending a source index column to the read table.
Checklist