Prepend source and row index columns in Hybrid scan reader - #22878
Prepend source and row index columns in Hybrid scan reader#22878rapids-bot[bot] merged 21 commits into
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
| _strings_to_categorical = false; | ||
| _options.use_jit_filter = false; | ||
| _options.case_sensitive_names = true; | ||
| _options.prepend_source_index_column = false; |
There was a problem hiding this comment.
The only new field, rest is all whitespace
| _options.decimal_width = options.get_decimal_width(); | ||
| _options.use_jit_filter = options.is_enabled_use_jit_filter(); | ||
| _options.case_sensitive_names = options.is_enabled_case_sensitive_names(); | ||
| _options.prepend_source_index_column = options.is_enabled_prepend_source_index_column(); |
There was a problem hiding this comment.
The only new field, rest is all whitespace
| out_metadata.num_rows_per_source = | ||
| std::vector<std::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.
Just remove the if condition (like regular reader) - rest is all whitespace
| out_metadata.num_rows_per_source = std::move(_file_itm_data.num_rows_per_source); | ||
| } | ||
| // Compute the output number of rows per source | ||
| if (mode == read_mode::CHUNKED_READ) { |
There was a problem hiding this comment.
Just remove the if condition (like regular reader) - rest is all whitespace
| apply_decimal_width_cast(out_columns); | ||
|
|
||
| // Prepend the source index column to filter columns or all columns | ||
| if (_options.prepend_source_index_column and |
There was a problem hiding this comment.
Only add source index column to filter columns (arbitrary design choice - could be payload as well)
| auto const num_prepended_cols = static_cast<size_type>(_options.prepend_source_index_column); | ||
| auto const final_filter = | ||
| parquet::detail::offset_column_references(_expr_conv.get_converted_expr(), num_prepended_cols); | ||
| auto const final_filter_expr = final_filter.get_converted_expr(); |
There was a problem hiding this comment.
Changes borrowed from the main reader (could be put in a common util 🤔 )
| _stream, | ||
| cudf::get_current_device_resource_ref()); | ||
| auto final_row_mask = cudf::detail::compute_column(*read_table, | ||
| final_filter_expr.value().get(), |
There was a problem hiding this comment.
Filter with final_filter_expr instead of _expr_conv
| * | ||
| * @return True if AST filter is not present | ||
| */ | ||
| [[nodiscard]] bool include_output_num_rows_per_source() const |
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:
📝 WalkthroughWalkthroughThis PR adds a ChangesPrepend source index column support
SPDX copyright header updates
Estimated code review effort: 3 (Moderate) | ~25 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 (1)
cpp/tests/io/experimental/hybrid_scan_test.cpp (1)
961-963: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
num_rows_per_sourcemetadata frommaterialize_*results is never checked.Cohort 2 of this PR stack made
out_metadata.num_rows_per_sourcecomputation unconditional inhybrid_scan_impl.cpp(previously gated oninclude_output_num_rows_per_source()) and removed the now-unused helper inreader_impl.hpp. This test discards the metadata (.tblonly) from everymaterialize_filter_columns_chunk,materialize_payload_columns_chunk, andmaterialize_all_columns_chunkcall, so the unconditional computation path introduced by that change is not exercised or validated here.Also applies to: 981-983, 1009-1011
🤖 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_test.cpp` around lines 961 - 963, The hybrid scan test is ignoring the `num_rows_per_source` metadata returned by `materialize_*` APIs, so the new unconditional metadata computation in `hybrid_scan_impl.cpp` is not being exercised. Update the test cases around `reader->materialize_filter_columns_chunk`, `reader->materialize_payload_columns_chunk`, and `reader->materialize_all_columns_chunk` to capture and assert the returned `out_metadata.num_rows_per_source` (not just `.tbl`), using the existing `reader`/`row_mask_view` setup to validate the metadata path.
🤖 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 886-889: The hybrid scan test currently uses only one source, so
it never validates source-index assignment or ordering. Update the test around
create_parquet_with_stats and finalize_output to use a multi-source case with
num_concat > 1, then assert the prepended source-index column contains the
expected values for each source rather than only checking column
placement/count. Keep the existing single-source case if needed, but add
coverage in hybrid_scan_test.cpp that exercises multiple row-group sources and
verifies the source-index differentiation logic directly.
---
Nitpick comments:
In `@cpp/tests/io/experimental/hybrid_scan_test.cpp`:
- Around line 961-963: The hybrid scan test is ignoring the
`num_rows_per_source` metadata returned by `materialize_*` APIs, so the new
unconditional metadata computation in `hybrid_scan_impl.cpp` is not being
exercised. Update the test cases around
`reader->materialize_filter_columns_chunk`,
`reader->materialize_payload_columns_chunk`, and
`reader->materialize_all_columns_chunk` to capture and assert the returned
`out_metadata.num_rows_per_source` (not just `.tbl`), using the existing
`reader`/`row_mask_view` setup to validate the metadata path.
🪄 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: c845e231-2ee9-432b-af4a-a383714596f4
📒 Files selected for processing (3)
cpp/src/io/parquet/experimental/hybrid_scan_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/tests/io/experimental/hybrid_scan_test.cpp
💤 Files with no reviewable changes (1)
- cpp/src/io/parquet/reader_impl.hpp
|
/ok to test 0967b9b |
mhaseeb123
added
5 - Ready to Merge
|
/merge |
4 participants
Description
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
Checklist