Prepend row index column in Parquet reader - #23077
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
changed the title
mhaseeb123
added
feature request
mhaseeb123
added
3 - Ready for Review
|
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:
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
WalkthroughAdds a ChangesRow index column feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (4)
cpp/tests/io/parquet_chunked_reader_test.cu (1)
1986-2126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Correct row-index/source-index arithmetic across chunked multi-source reads.
Verified the
i / num_rows/i % num_rowsreconstruction for unfiltered, skip/num_rows, and filtered cases against the multi-source concatenation and per-source filtering semantics — all consistent with the documentedprepend_row_index_column/prepend_source_index_columncontract.One gap: none of the three scenarios exercise a source with null values in the data column, or a source fully excluded by the filter (zero surviving rows). Given this is new device-synthesis logic for row/source index columns interacting with filtering and chunked multi-pass reads, adding at least one case with nulls and one with a fully-filtered-out source would strengthen coverage.
As per coding guidelines, "Tests missing edge cases: empty input, null values, sliced columns, boundary sizes, multi-block sizes" should be covered for changed/critical paths.
🤖 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/parquet_chunked_reader_test.cu` around lines 1986 - 2126, The new multi-source chunked parquet test covers the happy paths but misses two important edge cases: nulls in the data column and a source that produces zero surviving rows after filtering. Update TestRowIndexColumnMultipleSources to add a scenario using null-containing input and another where one source is fully filtered out, and verify prepend_source_index_column and prepend_row_index_column still produce correct results for those cases.Source: Coding guidelines
cpp/tests/io/parquet_reader_test.cpp (1)
4981-5146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! Solid coverage of row-index semantics, including the tricky out-of-order row-group case.
Traced through both tests:
RowIndexColumncorrectly validates per-source reset behavior, column ordering (row_idxalone vs.src_idx, row_idx, data), and filtered-row-index recomputation.RowIndexSelectedRead's out-of-order row-group test (Lines 5108-5119) is a particularly valuable check that file-local row index reflects absolute file position rather than selection/read order.As with the chunked-reader tests, coverage is missing for a data column containing nulls and for a source that is entirely filtered out (zero surviving rows) when both index columns are enabled. Worth adding given this is new logic mapping filtered/selected rows back to absolute file-local positions.
As per coding guidelines, "Tests missing edge cases: empty input, null values, sliced columns, boundary sizes, multi-block sizes" should be covered for changed/critical paths.
🤖 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/parquet_reader_test.cpp` around lines 4981 - 5146, Add missing edge-case coverage in the ParquetReaderTest row-index tests: extend RowIndexColumn and/or RowIndexSelectedRead to verify row-index behavior when the data column contains nulls and when a source is completely filtered out with both prepend_source_index_column and prepend_row_index_column enabled. Use the existing test helpers and symbols like RowIndexColumn, RowIndexSelectedRead, read_parquet, parquet_reader_options::builder, and the filter/row_groups setup to assert the index columns still map to file-local positions or produce an empty result as expected.Source: Coding guidelines
cpp/include/cudf/io/parquet.hpp (1)
316-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the row index column's data type.
The doc explains ordering but not the resulting column type. Per the device-side implementation (
synthesize_row_index_columninreader_impl_preprocess.cu), the row index column usessize_t(UINT64), whereas the sibling source index column usescudf::size_type(INT32). Since callers rely on this doc to know what type to expect downstream, documenting the dtype asymmetry here would help avoid confusion.📝 Proposed doc addition
/** * `@brief` Returns whether to prepend a file-local row index column to the output. * * The row index column contains, for each output row, the row's index within its parquet * source file. If the source index column is also enabled, the column order is: source index, * row index, data columns. * + * `@note` The row index column has type `size_t` (mapped to `UINT64`), unlike the source index + * column which uses `cudf::size_type` (`INT32`). + * * `@return` `true` if a row index column should be prepended */ [[nodiscard]] bool is_enabled_prepend_row_index_column() const🤖 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/include/cudf/io/parquet.hpp` around lines 316 - 329, The getter is documented for ordering but not for the row index column’s actual dtype, which is different from the source index column. Update the doc comment on is_enabled_prepend_row_index_column to explicitly state that the synthesized row index column uses size_t/UINT64, while the source index column uses cudf::size_type/INT32, so callers know what type to expect downstream.cpp/src/io/parquet/reader_impl.cpp (1)
880-896: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate pass-in-bounds condition.
_file_itm_data._current_input_pass < _file_itm_data.num_passes()is evaluated twice back-to-back (once forread_info, once for the chunk-advance guard). Hoisting into a local avoids risk of the two checks drifting apart in future edits.♻️ Proposed dedup
- // Row-range of the current output chunk relative to the current row group selection. - auto const read_info = - (_file_itm_data._current_input_pass < _file_itm_data.num_passes()) - ? _pass_itm_data->subpass - ->output_chunk_read_info[_pass_itm_data->subpass->current_output_chunk] - : row_range{0, 0}; - - // advance output chunk/subpass/pass info for non-empty tables if and only if we are in bounds - if (_file_itm_data._current_input_pass < _file_itm_data.num_passes()) { + auto const has_current_pass = _file_itm_data._current_input_pass < _file_itm_data.num_passes(); + + // Row-range of the current output chunk relative to the current row group selection. + auto const read_info = + has_current_pass + ? _pass_itm_data->subpass + ->output_chunk_read_info[_pass_itm_data->subpass->current_output_chunk] + : row_range{0, 0}; + + // advance output chunk/subpass/pass info for non-empty tables if and only if we are in bounds + if (has_current_pass) { auto& pass = *_pass_itm_data; auto& subpass = *pass.subpass; subpass.current_output_chunk++; }🤖 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.cpp` around lines 880 - 896, The pass-in-bounds check in reader_impl.cpp is duplicated for both computing read_info and advancing the output chunk state, so hoist `_file_itm_data._current_input_pass < _file_itm_data.num_passes()` into a single local and reuse it in this section. Update the logic around `read_info`, `pass.subpass`, and `subpass.current_output_chunk` to rely on that shared boolean so the guard stays consistent and does not drift in future edits.
🤖 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_preprocess.cu`:
- Around line 1219-1231: The row-offset staging buffer in the preprocess path
can be destroyed before the async H2D copy finishes. Update the block that
builds `host_row_offsets` and calls `cudf::detail::make_device_uvector_async` /
`cudf::detail::label_segments` to keep the pinned buffer alive until the stream
work is complete, matching the `synthesize_row_index_column` pattern by
synchronizing `_stream` before the block exits.
---
Nitpick comments:
In `@cpp/include/cudf/io/parquet.hpp`:
- Around line 316-329: The getter is documented for ordering but not for the row
index column’s actual dtype, which is different from the source index column.
Update the doc comment on is_enabled_prepend_row_index_column to explicitly
state that the synthesized row index column uses size_t/UINT64, while the source
index column uses cudf::size_type/INT32, so callers know what type to expect
downstream.
In `@cpp/src/io/parquet/reader_impl.cpp`:
- Around line 880-896: The pass-in-bounds check in reader_impl.cpp is duplicated
for both computing read_info and advancing the output chunk state, so hoist
`_file_itm_data._current_input_pass < _file_itm_data.num_passes()` into a single
local and reuse it in this section. Update the logic around `read_info`,
`pass.subpass`, and `subpass.current_output_chunk` to rely on that shared
boolean so the guard stays consistent and does not drift in future edits.
In `@cpp/tests/io/parquet_chunked_reader_test.cu`:
- Around line 1986-2126: The new multi-source chunked parquet test covers the
happy paths but misses two important edge cases: nulls in the data column and a
source that produces zero surviving rows after filtering. Update
TestRowIndexColumnMultipleSources to add a scenario using null-containing input
and another where one source is fully filtered out, and verify
prepend_source_index_column and prepend_row_index_column still produce correct
results for those cases.
In `@cpp/tests/io/parquet_reader_test.cpp`:
- Around line 4981-5146: Add missing edge-case coverage in the ParquetReaderTest
row-index tests: extend RowIndexColumn and/or RowIndexSelectedRead to verify
row-index behavior when the data column contains nulls and when a source is
completely filtered out with both prepend_source_index_column and
prepend_row_index_column enabled. Use the existing test helpers and symbols like
RowIndexColumn, RowIndexSelectedRead, read_parquet,
parquet_reader_options::builder, and the filter/row_groups setup to assert the
index columns still map to file-local positions or produce an empty result as
expected.
🪄 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: 0ab2afa8-f5b7-4504-ac18-2eaff3111df1
📒 Files selected for processing (8)
cpp/include/cudf/io/parquet.hppcpp/src/io/parquet/reader_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/src/io/parquet/reader_impl_helpers.cppcpp/src/io/parquet/reader_impl_helpers.hppcpp/src/io/parquet/reader_impl_preprocess.cucpp/tests/io/parquet_chunked_reader_test.cucpp/tests/io/parquet_reader_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/io/parquet_reader_test.cpp (1)
4981-5155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd edge-case coverage for the new row_index tests.
The new
RowIndexColumn,SourceAndRowIndexColumns, andRowIndexSelectedReadtests exercise multi-source, filtered, and row-group-selection scenarios well, but none cover an empty source/table, a source containing nulls, or a sliced input column withprepend_row_index_column. Consider adding at least one case for an empty parquet file/read and one with nullable data columns to confirm the row_index column is still correctly synthesized (or trivially empty) in those situations.Based on path instructions ("Test suites should cover edge cases such as empty input, null values, sliced columns, boundary sizes, and multi-block sizes.").
🤖 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/parquet_reader_test.cpp` around lines 4981 - 5155, The new row_index coverage is missing edge cases for empty input and nullable/sliced data, so extend the existing ParquetReaderTest cases in RowIndexColumn, SourceAndRowIndexColumns, or RowIndexSelectedRead to include at least one empty parquet read and one nullable column scenario, plus a sliced input with prepend_row_index_column. Verify the synthesized row_index column is empty when appropriate and remains correctly offset/restarted for null-containing and sliced inputs.Source: Path instructions
🤖 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.
Nitpick comments:
In `@cpp/tests/io/parquet_reader_test.cpp`:
- Around line 4981-5155: The new row_index coverage is missing edge cases for
empty input and nullable/sliced data, so extend the existing ParquetReaderTest
cases in RowIndexColumn, SourceAndRowIndexColumns, or RowIndexSelectedRead to
include at least one empty parquet read and one nullable column scenario, plus a
sliced input with prepend_row_index_column. Verify the synthesized row_index
column is empty when appropriate and remains correctly offset/restarted for
null-containing and sliced inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5d215cb-88ff-45e3-8687-2a129f8a1d57
📒 Files selected for processing (3)
cpp/src/io/parquet/reader_impl.cppcpp/src/io/parquet/reader_impl_preprocess.cucpp/tests/io/parquet_reader_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/io/parquet/reader_impl.cpp
- cpp/src/io/parquet/reader_impl_preprocess.cu
| column_name_info{.name = "src_idx", .is_nullable = false}); | ||
| // Prepend the source and row index columns if requested | ||
| { | ||
| if (_options.prepend_row_index_column) { |
There was a problem hiding this comment.
Synthesize and prepend row index column
| std::vector<size_t> aggregate_reader_metadata::compute_source_row_group_offsets( | ||
| size_type src_idx) const | ||
| { | ||
| CUDF_EXPECTS(src_idx >= 0 && std::cmp_less(src_idx, per_file_metadata.size()), |
There was a problem hiding this comment.
Compute absolute (file-local) row offset for each row group
|
|
||
| } // namespace | ||
|
|
||
| std::unique_ptr<column> reader_impl::synthesize_row_index_column(row_range const& read_info) |
There was a problem hiding this comment.
This function is identical to synthesize_source_index_column except the computation functor above. I did consider a common function taking in a templated functor but it didn't look any better than duplicate looking functions
There was a problem hiding this comment.
can you at least factor out common code into utility functions?
There was a problem hiding this comment.
Common code is barely a couple lines. Not sure if it's worth it.
…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.
mhaseeb123
added
5 - Ready to Merge
|
/merge |
Follow up #23077 Supply stream and mr to column synthesizers which are used to produce the output columns Authors: - Muhammad Haseeb (https://github.com/mhaseeb123) Approvers: - Bradley Dice (https://github.com/bdice) - David Wendt (https://github.com/davidwendt) URL: #23209
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
4 participants
Description
Contributes to #22849
This PR enables optionally prepending a file-local row index (i.e. the absolute index of each row in the file it came from) column to the read table.
Checklist