refactor(parquet): introduce string_offset_scan_state for preprocess_string_offsets_kernel by vyasr · Pull Request #23495 · NVIDIA/cudf

vyasr · 2026-07-31T04:52:10Z

Description

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

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…rocess_string_offsets_kernel via string_offset_scan_state

coderabbitai · 2026-07-31T04:55:30Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 75716962-9d71-4041-b107-d3dc467244bf

📥 Commits

Reviewing files that changed from the base of the PR and between 43ca9a2 and 03f9c78.

📒 Files selected for processing (1)

cpp/src/io/parquet/page_state_composed.cuh

🚧 Files skipped from review as they are similar to previous changes (1)

cpp/src/io/parquet/page_state_composed.cuh

📝 Walkthrough

Summary by CodeRabbit

Bug Fixes
- Improved Parquet decoding reliability for fixed-width, string, dictionary-encoded, boolean, delta-encoded, and nested data.
- Improved handling of page boundaries, row limits, validity information, and list data.
- Improved string offset processing and page containment checks for more consistent results.
- Improved source-position tracking and value-count handling during dictionary and delta decoding.

Walkthrough

Parquet decoding now stores cumulative counters and row-bound state in page_state_s::progress. Page-boundary helpers use page state directly. Fixed-width, page, delta, and string decoders were updated. String offset preprocessing now uses a composed state.

Changes

Parquet progress-state migration

Layer / File(s)	Summary
Progress state and page-boundary contracts `cpp/src/io/parquet/page_decode.cuh`	Adds `page_decode_progress_state`, nests progress in `page_state_s`, updates boundary helpers, and initializes progress fields.
Fixed-width decoding and preprocessing `cpp/src/io/parquet/decode_fixed.cu`, `cpp/src/io/parquet/decode_preprocess.cu`	Routes validity, row, value, leaf, dictionary, and row-bound counters through `s->progress`.
Page and delta decoder progress `cpp/src/io/parquet/page_data.cu`, `cpp/src/io/parquet/page_delta_decode.cu`	Updates page and delta decoders to use nested source-position, dictionary-position, and input counters.
String offset scan state `cpp/src/io/parquet/page_state_composed.cuh`, `cpp/src/io/parquet/page_string_decode.cu`	Adds `string_offset_scan_state` and updates string offset readers and page-boundary calls to use composed state.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related PRs

rapidsai/cudf#23241: Refactors the same Parquet page-boundary helpers and preprocessing and string-decoding call sites.
rapidsai/cudf#23374: Overlaps in the Parquet page-state and decoder paths modified by this change.
rapidsai/cudf#23471: Refactors page_state_s access across the same Parquet decoder kernels.

Suggested reviewers: bdice, mythrocks

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary change: introducing string_offset_scan_state for Parquet string-offset preprocessing.
Description check	✅ Passed	The description accurately explains the state refactoring, kernel change, and shared-memory reduction.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7f58752 and 4f6848d.

📒 Files selected for processing (7)

cpp/src/io/parquet/decode_fixed.cu
cpp/src/io/parquet/decode_preprocess.cu
cpp/src/io/parquet/page_data.cu
cpp/src/io/parquet/page_decode.cuh
cpp/src/io/parquet/page_delta_decode.cu
cpp/src/io/parquet/page_state_composed.cuh
cpp/src/io/parquet/page_string_decode.cu

bdice · 2026-07-31T06:35:42Z

+  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");


Is this meant to be left in?

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.

vyasr · 2026-08-10T15:47:32Z

/merge

…_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.

…_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

refactor(parquet): extract page_decode_progress_state and narrow prep…

4f6848d

…rocess_string_offsets_kernel via string_offset_scan_state

vyasr requested a review from a team as a code owner July 31, 2026 04:52

vyasr requested review from bdice and mythrocks July 31, 2026 04:52

github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Jul 31, 2026

github-actions Bot assigned vyasr Jul 31, 2026

vyasr added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 31, 2026

coderabbitai Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread cpp/src/io/parquet/page_decode.cuh

Comment thread cpp/src/io/parquet/page_state_composed.cuh Outdated

vyasr added 2 commits July 31, 2026 05:10

refactor(parquet): address string offset scan review comments

43ca9a2

Update struct description

03f9c78

coderabbitai Bot mentioned this pull request Jul 31, 2026

refactor(parquet): introduce string_size_scan_state for string size kernels #23496

Merged

3 tasks

bdice approved these changes Jul 31, 2026

View reviewed changes

mhaseeb123 approved these changes Jul 31, 2026

View reviewed changes

rapids-bot Bot merged commit 1474ca0 into NVIDIA:main Aug 10, 2026
134 of 136 checks passed

vyasr deleted the parquet-substruct-redesign/pr4-progress-string-offset branch August 10, 2026 15:47

vyasr mentioned this pull request Aug 10, 2026

refactor(parquet): extract page_decode_nesting_state and compose full_page_decode_state replacing page_state_s #23610

Merged

3 tasks

Navigation Menu

Uh oh!

refactor(parquet): introduce string_offset_scan_state for preprocess_string_offsets_kernel - #23495

refactor(parquet): introduce string_offset_scan_state for preprocess_string_offsets_kernel#23495
rapids-bot[bot] merged 3 commits into
NVIDIA:mainfrom
vyasr:parquet-substruct-redesign/pr4-progress-string-offset

vyasr commented Jul 31, 2026

Uh oh!

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

bdice Jul 31, 2026

Uh oh!

vyasr Jul 31, 2026

Uh oh!

vyasr commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

vyasr commented Jul 31, 2026

Description

Checklist

Uh oh!

coderabbitai Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bdice Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

vyasr Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

vyasr commented Aug 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading