refactor(parquet): introduce string_offset_scan_state for preprocess_string_offsets_kernel - #23495
Conversation
…rocess_string_offsets_kernel via string_offset_scan_state
|
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
WalkthroughParquet decoding now stores cumulative counters and row-bound state in ChangesParquet progress-state migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/page_decode.cuh`:
- Around line 204-207: Update the Doxygen blocks for is_bounds_page and the
other affected helper declarations to document the actual s parameter, replacing
the obsolete page and chunk_start_row entries. Ensure every documented parameter
matches its corresponding signature, including start_row, num_rows, and
has_repetition.
In `@cpp/src/io/parquet/page_state_composed.cuh`:
- Around line 47-50: Update the static_assert involving string_offset_scan_state
to compare its size directly against sizeof(page_state_s), removing the
additional page_decode_* size terms so the assertion detects shared-memory size
regressions.
🪄 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: aeb3f98f-cc50-4e63-8a04-2c5908743ce3
📒 Files selected for processing (7)
cpp/src/io/parquet/decode_fixed.cucpp/src/io/parquet/decode_preprocess.cucpp/src/io/parquet/page_data.cucpp/src/io/parquet/page_decode.cuhcpp/src/io/parquet/page_delta_decode.cucpp/src/io/parquet/page_state_composed.cuhcpp/src/io/parquet/page_string_decode.cu
| CUDF_PARQUET_PAGE_STATE_ERROR_METHODS | ||
| }; | ||
| static_assert(sizeof(string_offset_scan_state) < sizeof(page_state_s), | ||
| "string_offset_scan_state did not shrink after removing output conversion state"); |
There was a problem hiding this comment.
Is this meant to be left in?
There was a problem hiding this comment.
It can be removed, but since CI already passed here unless another reviewer requests changes I'll just leave it for now and clean it up in a follow-up PR. We'll be either renaming/removing page_state_s so this'll have to change then.
Description
This PR builds on #23479 to continue narrowing the shared state used by Parquet preprocessing kernels. It extracts
page_decode_progress_statefrompage_state_s, grouping the decode-position/progress fields that are needed by string-offset preprocessing.The primary addition is
string_offset_scan_state, composed frompage_decode_setup_state,page_decode_stream_state, andpage_decode_progress_state.preprocess_string_offsets_kernelnow uses this smaller composed state instead of the fullpage_state_s, which drops unrelated output-conversion, nesting, and level-scratch fields from that kernel's shared-memory state. This follows the same pattern as #23479: pull one coherent sub-state out ofpage_state_s, migrate the target kernel to the minimal composed state it actually needs, and leave the remaining full decode state for a later PR.A fresh
cuobjdump -res-usagecomparison againstupstream/mainshowspreprocess_string_offsets_kernelsaves 512 bytes of shared memory on every generated architecture, with no register-count changes:Checklist