refactor(parquet): extract page_decode_nesting_state and compose full_page_decode_state replacing page_state_s - #23610
Conversation
…_page_decode_state replacing page_state_s
|
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR decomposes Parquet page decode state into composable structures. Decode and preprocessing helpers now accept generic state pointers. Nesting metadata uses the nested state layout. Bounds checks receive explicit page and row metadata. ChangesParquet page state refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/io/parquet/page_decode.cuh (1)
1285-1292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate
data_outassignment.Line 1286 and Line 1292 store the same value into
nesting_info->data_out. The second store is dead. It also suggests that thestring_outassignment at Line 1288 depends on the order of the two stores, which it does not.♻️ Proposed cleanup
if (s->setup.col.column_data_base != nullptr) { nesting_info->data_out = static_cast<uint8_t*>(s->setup.col.column_data_base[idx]); if (s->setup.col.column_string_base != nullptr) { nesting_info->string_out = static_cast<uint8_t*>(s->setup.col.column_string_base[idx]); } - nesting_info->data_out = static_cast<uint8_t*>(s->setup.col.column_data_base[idx]); - if (nesting_info->data_out != nullptr) {🤖 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/page_decode.cuh` around lines 1285 - 1292, Remove the redundant second assignment to nesting_info->data_out in the column data initialization block, while preserving the first assignment and the conditional nesting_info->string_out assignment.
🤖 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 184-190: Update the Doxygen parameter lists for is_bounds_page,
is_page_contained, and page_has_rows_to_process to document PageInfo const& page
and size_t chunk_start_row, and remove the obsolete `@param` s entry. Keep the
remaining parameter descriptions accurate and ensure every parameter in each
signature is documented.
---
Nitpick comments:
In `@cpp/src/io/parquet/page_decode.cuh`:
- Around line 1285-1292: Remove the redundant second assignment to
nesting_info->data_out in the column data initialization block, while preserving
the first assignment and the conditional nesting_info->string_out assignment.
🪄 Autofix
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: 117517d7-63cf-42d6-83c0-a7684063f1a6
📒 Files selected for processing (9)
cpp/src/io/parquet/decode_fixed.cucpp/src/io/parquet/decode_preprocess.cucpp/src/io/parquet/page_data.cucpp/src/io/parquet/page_data.cuhcpp/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.cucpp/src/io/parquet/page_string_utils.cuh
Address review feedback on NVIDIA#23610: the doc blocks for is_bounds_page, is_page_contained, and page_has_rows_to_process still documented the old `@param s` after the refactor changed their signatures to take `PageInfo const& page` and `size_t chunk_start_row`. Doxygen is used as a documentation linter, so undocumented/nonexistent params were flagged. No functional change.
|
/merge |
Description
This PR completes the incremental narrowing pattern from #23471, #23479, #23495, and #23496 by retiring
page_state_sentirely. It extractspage_decode_nesting_state(the nesting decode cache and pointer) and composesfull_page_decode_statefrompage_decode_setup_state,page_decode_stream_state,page_decode_nesting_state,page_decode_progress_state, andpage_decode_output_state. Every full-decode kernel (decode_page_data,decode_split_page_data_kernel,decode_page_data_generic,decode_delta_binary,decode_delta_byte_array,decode_delta_length_byte_array,compute_string_page_bounds,compute_page_sizes) now takes this composed state instead of the ad-hocpage_state_s, and the old struct is deleted.The composition also folds
page_state_s's stand-alone level-decoding fields (lvl_start[2],lvl_end) into the already-existingpage_decode_stream_state::abs_lvl_start/abs_lvl_endarrays, and drops the unusedfirst_output_valuefield. Together with the composition's tighter layout this removes 32 bytes of shared memory from every full-decode kernel on every generated architecture, with no register-count regressions.A fresh
cuobjdump -res-usagecomparison againstupstream/mainshows the same 32-byte shmem reduction on all eight migrated kernels on every generated architecture. Register usage does not increase anywhere; one variant ofdecode_split_page_data_kernelonsm_86drops by 8 registers:decode_page_datadecode_split_page_data_kernelsm_86: one template variant 48 -> 40decode_page_data_genericcompute_page_sizescompute_string_page_boundsdecode_delta_binarydecode_delta_byte_arraydecode_delta_length_byte_arrayRepresentative sm_80 measurements:
decode_page_datadecode_split_page_data_kerneldecode_page_data_generic(min variant)decode_page_data_generic(max variant)compute_page_sizescompute_string_page_boundsdecode_delta_binarydecode_delta_byte_arraydecode_delta_length_byte_arrayWith this PR the
page_state_stype is fully removed. All Parquet decode and preprocess kernels now use one of four purpose-built shared-memory states (level_scan_state,string_size_scan_state,string_offset_scan_state,full_page_decode_state), each holding only the substructs it actually needs.Checklist