refactor(parquet): introduce level_state_scan struct for preprocess_levels_kernel - #23479
Conversation
…cess_levels_kernel shmem via level_scan_state
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe Parquet decoder now composes page setup and stream state, updates level preprocessing to use the composed state, and migrates fixed-width, dictionary, DELTA, and string decoding paths to stream-backed pointers and metadata. Parquet stream state refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/decode_preprocess.cu (1)
446-460: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard decoder init when
s->setup.erroris set. Malformed pages can set an error whileInitLevelSectionleavess->stream.abs_lvl_start[lvl]unset, andsetup_local_page_infostill reaches bothdecoders[...].init(...)calls. Skip the init/decode path or return early so the RLE decoder never sees an indeterminate level-stream pointer.🤖 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/decode_preprocess.cu` around lines 446 - 460, Guard the repetition-level decoder setup in the shown preprocessing flow with s->setup.error before invoking decoders[level_type::REPETITION].init or decode_next. When an error is set, skip the barrier/init/decode path or return early, ensuring no decoder receives an unset s->stream.abs_lvl_start pointer; preserve normal processing when no error exists.
🧹 Nitpick comments (3)
cpp/src/io/parquet/page_state_composed.cuh (2)
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit
constexprdefault constructor, mirroringpage_state_s.
level_scan_state's members carry NSDMIs (page_decode_setup_state setup{}, arrays inpage_decode_stream_state), so the__shared__ level_scan_state state_g;indecode_preprocess.cuis only accepted because of the#pragma nv_diag_suppress static_var_with_dynamic_initabove the kernel.page_state_sdeliberately declaresCUDF_HOST_DEVICE constexpr page_state_s() noexcept {}for this reason; doing the same here keeps the shared declaration valid without relying on the suppression, and avoids the (dead) shared-memory zero-init intent.♻️ Proposed change
struct level_scan_state { + CUDF_HOST_DEVICE constexpr level_scan_state() noexcept {} page_decode_setup_state setup; page_decode_stream_state stream; CUDF_PARQUET_PAGE_STATE_ERROR_METHODS };🤖 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_state_composed.cuh` around lines 29 - 33, Update the level_scan_state struct with an explicit CUDF_HOST_DEVICE constexpr noexcept default constructor, matching page_state_s, while preserving its existing members and error methods. This should make its default construction explicit for shared-memory use without relying on the diagnostic suppression.
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse doxygen comments for the new struct.
level_scan_stateis a new public-in-namespace type documented with plain//comments; a/**@brief... */block keeps it consistent with the doxygen tooling used for the rest of the C++/CUDA sources.As per coding guidelines: "Use doxygen as a documentation generator and linter for C++ and CUDA code."
🤖 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_state_composed.cuh` around lines 26 - 33, Replace the plain `//` documentation above `level_scan_state` with a Doxygen block comment using the project’s standard `/** `@brief` ... */` style, while preserving the existing description of the struct’s setup and stream state.Source: Coding guidelines
cpp/src/io/parquet/page_decode.cuh (1)
128-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider constraining the
auto*parameters.Switching to
auto*makes these helpers usable with bothpage_state_sandlevel_scan_state, but it also silently accepts anything with asetupmember and moves diagnostics to the point of use. A small concept (e.g.requires { s->setup.col; s->stream.initial_rle_run; }) or a named template parameter would document the contract these helpers actually rely on.Also applies to: 147-163, 171-174
🤖 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 128 - 133, Constrain the auto* parameters in is_nullable and the related helpers at lines 147-163 and 171-174 to the intended page_state_s/level_scan_state contract. Add a shared concept or named template requirement covering the setup.col and stream.initial_rle_run members those helpers use, so unrelated types are rejected at definition time.
🤖 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/decode_preprocess.cu`:
- Around line 446-460: Guard the repetition-level decoder setup in the shown
preprocessing flow with s->setup.error before invoking
decoders[level_type::REPETITION].init or decode_next. When an error is set, skip
the barrier/init/decode path or return early, ensuring no decoder receives an
unset s->stream.abs_lvl_start pointer; preserve normal processing when no error
exists.
---
Nitpick comments:
In `@cpp/src/io/parquet/page_decode.cuh`:
- Around line 128-133: Constrain the auto* parameters in is_nullable and the
related helpers at lines 147-163 and 171-174 to the intended
page_state_s/level_scan_state contract. Add a shared concept or named template
requirement covering the setup.col and stream.initial_rle_run members those
helpers use, so unrelated types are rejected at definition time.
In `@cpp/src/io/parquet/page_state_composed.cuh`:
- Around line 29-33: Update the level_scan_state struct with an explicit
CUDF_HOST_DEVICE constexpr noexcept default constructor, matching page_state_s,
while preserving its existing members and error methods. This should make its
default construction explicit for shared-memory use without relying on the
diagnostic suppression.
- Around line 26-33: Replace the plain `//` documentation above
`level_scan_state` with a Doxygen block comment using the project’s standard
`/** `@brief` ... */` style, while preserving the existing description of the
struct’s setup and stream state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3579ebe1-a151-442b-954c-dd74403c1978
📒 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
|
/merge |
…string_offsets_kernel (#23495) This PR builds on #23479 to continue narrowing the shared state used by Parquet preprocessing kernels. It extracts `page_decode_progress_state` from `page_state_s`, grouping the decode-position/progress fields that are needed by string-offset preprocessing. The primary addition is `string_offset_scan_state`, composed from `page_decode_setup_state`, `page_decode_stream_state`, and `page_decode_progress_state`. `preprocess_string_offsets_kernel` now uses this smaller composed state instead of the full `page_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 of `page_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-usage` comparison against `upstream/main` shows `preprocess_string_offsets_kernel` saves 512 bytes of shared memory on every generated architecture, with no register-count changes: | Arch | shmem before | shmem after | delta | regs before | regs after | |---|---:|---:|---:|---:|---:| | sm_70 | 2048 | 1536 | -512 | 40 | 40 | | sm_75 | 2048 | 1536 | -512 | 64 | 64 | | sm_80 | 2048 | 1536 | -512 | 37 | 37 | | sm_86 | 2048 | 1536 | -512 | 38 | 38 | | sm_90a | 3072 | 2560 | -512 | 32 | 32 | | sm_100f | 3072 | 2560 | -512 | 32 | 32 | | sm_120a | 3072 | 2560 | -512 | 40 | 40 | | sm_120 | 3072 | 2560 | -512 | 40 | 40 | Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #23495
…ernels (#23496) This PR builds on #23479 to create a minimal shared state for the Parquet string-size scan kernels. It extracts `page_decode_output_state` from `page_state_s`, grouping the output conversion fields (`dtype_len`, `dtype_len_in`, and `ts_scale`) that are used while computing string output sizes. The primary addition is `string_size_scan_state`, composed from `page_decode_setup_state`, `page_decode_stream_state`, and `page_decode_output_state`. The three string-size scan kernels (`compute_page_string_sizes_kernel`, `compute_delta_page_string_sizes_kernel`, and `compute_delta_length_page_string_sizes_kernel`) now use this smaller composed state instead of the full `page_state_s`. That removes unrelated nesting and progress fields from these kernels' shared-memory state while preserving the output-conversion metadata they need. A fresh `cuobjdump -res-usage` comparison against `upstream/main` shows shared-memory reductions in all three migrated kernels. Register usage does not increase anywhere; it drops on a few newer architectures: | Kernel | Arch summary | shmem delta | register changes | |---|---|---:|---| | `compute_page_string_sizes_kernel` | all generated arch variants | -536 bytes | unchanged except `sm_100f`: 32 -> 31 | | `compute_delta_page_string_sizes_kernel` | all generated arch variants | -536 bytes | unchanged except `sm_90a`: 40 -> 32, `sm_120a`: 48 -> 40, `sm_120`: 48 -> 40 | | `compute_delta_length_page_string_sizes_kernel` | all generated arch variants | -544 bytes | unchanged | Representative sm80 measurements: | Kernel | shmem before | shmem after | delta | regs before | regs after | |---|---:|---:|---:|---:|---:| | `compute_page_string_sizes_kernel` | 1160 | 624 | -536 | 40 | 40 | | `compute_delta_page_string_sizes_kernel` | 3528 | 2992 | -536 | 32 | 32 | | `compute_delta_length_page_string_sizes_kernel` | 2144 | 1600 | -544 | 62 | 62 | This is the same incremental pattern as #23479: extract one coherent sub-state, migrate the kernels that can use the narrower composition, and keep the remaining full decode state for the follow-up integration PR. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #23496
…_page_decode_state replacing page_state_s (#23610) This PR completes the incremental narrowing pattern from #23471, #23479, #23495, and #23496 by retiring `page_state_s` entirely. It extracts `page_decode_nesting_state` (the nesting decode cache and pointer) and composes `full_page_decode_state` from `page_decode_setup_state`, `page_decode_stream_state`, `page_decode_nesting_state`, `page_decode_progress_state`, and `page_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-hoc `page_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-existing `page_decode_stream_state::abs_lvl_start` / `abs_lvl_end` arrays, and drops the unused `first_output_value` field. 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-usage` comparison against `upstream/main` shows the same 32-byte shmem reduction on all eight migrated kernels on every generated architecture. Register usage does not increase anywhere; one variant of `decode_split_page_data_kernel` on `sm_86` drops by 8 registers: | Kernel | Arch summary | shmem delta | register changes | |---|---|---:|---| | `decode_page_data` | all generated arch variants | -32 bytes | unchanged | | `decode_split_page_data_kernel` | all generated arch variants | -32 bytes | unchanged except `sm_86`: one template variant 48 -> 40 | | `decode_page_data_generic` | all generated arch variants (44 template variants) | -32 bytes | unchanged | | `compute_page_sizes` | all generated arch variants | -32 bytes | unchanged | | `compute_string_page_bounds` | all generated arch variants | -32 bytes | unchanged | | `decode_delta_binary` | all generated arch variants | -32 bytes | unchanged | | `decode_delta_byte_array` | all generated arch variants | -32 bytes | unchanged | | `decode_delta_length_byte_array` | all generated arch variants | -32 bytes | unchanged | Representative sm_80 measurements: | Kernel | shmem before | shmem after | delta | regs before | regs after | |---|---:|---:|---:|---:|---:| | `decode_page_data` | 4112 | 4080 | -32 | 56 | 56 | | `decode_split_page_data_kernel` | 4112 | 4080 | -32 | 56 | 56 | | `decode_page_data_generic` (min variant) | 2104 | 2072 | -32 | 62 | 62 | | `decode_page_data_generic` (max variant) | 13024 | 12992 | -32 | 64 | 64 | | `compute_page_sizes` | 3240 | 3208 | -32 | 32 | 32 | | `compute_string_page_bounds` | 3256 | 3224 | -32 | 32 | 32 | | `decode_delta_binary` | 2700 | 2668 | -32 | 56 | 56 | | `decode_delta_byte_array` | 5068 | 5036 | -32 | 72 | 72 | | `decode_delta_length_byte_array` | 3392 | 3360 | -32 | 64 | 64 | With this PR the `page_state_s` type 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. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #23610
Description
This PR builds on #23471 to create a minimal struct for the
preprocess_levelskernel. The primary addition in this PR ispage_decode_stream_statestruct encapsulating the level stream decoding components ofpage_state_s, which now has one of those structs as a member.preprocess_levelsnow uses a minimallevel_scan_statestruct, which only contains the two sub-structs required by that kernel. The net result is a savings of 560 bytes of shared memory in the kernel. Interestingly, on a couple of architectures (sm75 and sm86) we also saw a drop register usage by 8 (80->72 on sm75, 54->46 on sm86). That is hopefully indicative of the ancillary benefits of reducing shared memory usage as we will be able to incrementally reduce the complexity of the kernels and make it easier for the compiler to produce better optimized code (in this case presumably with better register reuse by eliding intermediate shared memory accesses that made live range counting harder).Checklist