refactor(parquet): introduce string_size_scan_state for string size kernels - #23496
Conversation
… size scan kernels via string_size_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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughParquet page decoding now stores conversion metadata in ChangesParquet output conversion state
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 |
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/page_data.cu (1)
84-95: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMove the malformed-page check before the division.
Line 85 computes
num_valuesby dividingdata_lenbys->output_cvt.dtype_len_inbefore the guard at line 88 checkss->output_cvt.dtype_len_in <= 0. Ifdtype_len_inis 0 for a malformed BYTE_STREAM_SPLIT page, this division executes first and causes an integer division by zero, which is undefined behavior in device code. The guard below is meant to catch exactly this case, but it runs too late.The sibling function
decode_fixed_width_split_valuesindecode_fixed.cuperforms the check before the division. Apply the same order here.🐛 Proposed fix: check before dividing
auto const data_len = cuda::std::distance(s->stream.data_start, s->stream.data_end); - auto const num_values = data_len / s->output_cvt.dtype_len_in; - - // Check malformed BYTE_STREAM_SPLIT pages - if (s->output_cvt.dtype_len_in <= 0 or data_len <= 0) { + + // Check malformed BYTE_STREAM_SPLIT pages + if (s->output_cvt.dtype_len_in <= 0 or data_len <= 0) { cg::invoke_one(block, [&]() { set_error(static_cast<kernel_error::value_type>(decode_error::INVALID_BYTE_STREAM_SPLIT_SIZE), error_code); }); return; } + + auto const num_values = data_len / s->output_cvt.dtype_len_in;🤖 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_data.cu` around lines 84 - 95, Move the malformed BYTE_STREAM_SPLIT validation in the surrounding decode function before calculating num_values, checking dtype_len_in and data_len before dividing data_len by dtype_len_in. Preserve the existing set_error and return behavior, matching the ordering used by decode_fixed_width_split_values.
🤖 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/page_data.cu`:
- Around line 84-95: Move the malformed BYTE_STREAM_SPLIT validation in the
surrounding decode function before calculating num_values, checking dtype_len_in
and data_len before dividing data_len by dtype_len_in. Preserve the existing
set_error and return behavior, matching the ordering used by
decode_fixed_width_split_values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5843b27c-4bfd-4769-ae37-5ae10d8035ef
📒 Files selected for processing (8)
cpp/src/io/parquet/decode_fixed.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
bdice
left a comment
There was a problem hiding this comment.
Everything seems fine except this comment block, which is a little strange.
…edesign/pr5-output-string-size # Conflicts: # cpp/src/io/parquet/decode_fixed.cu # cpp/src/io/parquet/page_decode.cuh # cpp/src/io/parquet/page_state_composed.cuh
…_assert Addresses review comment on NVIDIA#23495: the sizeof(string_offset_scan_state) < sizeof(page_state_s) static_assert was verification scaffold from the initial struct extraction. Neither level_scan_state nor string_size_scan_state have an equivalent assert, and the shmem win is already covered by the resource-usage measurements in the PR description. Drop it for consistency.
|
/merge |
…_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 #23479 to create a minimal shared state for the Parquet string-size scan kernels. It extracts
page_decode_output_statefrompage_state_s, grouping the output conversion fields (dtype_len,dtype_len_in, andts_scale) that are used while computing string output sizes.The primary addition is
string_size_scan_state, composed frompage_decode_setup_state,page_decode_stream_state, andpage_decode_output_state. The three string-size scan kernels (compute_page_string_sizes_kernel,compute_delta_page_string_sizes_kernel, andcompute_delta_length_page_string_sizes_kernel) now use this smaller composed state instead of the fullpage_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-usagecomparison againstupstream/mainshows shared-memory reductions in all three migrated kernels. Register usage does not increase anywhere; it drops on a few newer architectures:compute_page_string_sizes_kernelsm_100f: 32 -> 31compute_delta_page_string_sizes_kernelsm_90a: 40 -> 32,sm_120a: 48 -> 40,sm_120: 48 -> 40compute_delta_length_page_string_sizes_kernelRepresentative sm80 measurements:
compute_page_string_sizes_kernelcompute_delta_page_string_sizes_kernelcompute_delta_length_page_string_sizes_kernelThis 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.
Checklist