Skip to content

patch for multipage row - #4828

Merged
pmattione-nvidia merged 2 commits into
NVIDIA:release/26.06from
pmattione-nvidia:patch_2606_list_string
Jul 16, 2026
Merged

pmattione-nvidia merged 2 commits into
NVIDIA:release/26.06from
pmattione-nvidia:patch_2606_list_string

Conversation

@pmattione-nvidia

Copy link
Copy Markdown
Collaborator

This adds a patch to release 26.06 for applying this fix for a parquet decoding bug in cuDF.

Signed-off-by: Paul Mattione <pmattione@nvidia.com>
@pmattione-nvidia
pmattione-nvidia requested a review from a team as a code owner July 15, 2026 22:25
@pmattione-nvidia pmattione-nvidia self-assigned this Jul 15, 2026
@pmattione-nvidia pmattione-nvidia added the bug Something isn't working label Jul 15, 2026
@pmattione-nvidia

Copy link
Copy Markdown
Collaborator Author

build

@pmattione-nvidia
pmattione-nvidia marked this pull request as draft July 15, 2026 22:28
@greptile-apps

greptile-apps Bot commented Jul 15, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR backports upstream cuDF fix rapidsai/cudf#23241 to the release/26.06 branch as a patch file, replacing the empty noop.patch placeholder. The fix addresses an out-of-bounds write during Parquet string-offset preprocessing caused by plain-encoded list<string> columns where a single list row spans multiple data pages, producing 0-row continuation pages that the host-side buffer sizing incorrectly skipped while GPU decode kernels still wrote offsets to them.

  • Core fix (reader_impl_preprocess.cu, page_decode.cuh): Introduces page_has_rows_to_process() to correctly identify 0-row list continuation pages as needing processing, replacing a simple row-range exclusion that failed for these pages. Refactors is_bounds_page and is_page_contained to accept PageInfo const& page + chunk_start_row directly, enabling reuse from both GPU and the preprocess-functor context.
  • Call-site updates (decode_preprocess.cu, page_delta_decode.cu, page_string_decode.cu): All existing call sites are updated to pass the explicit page and chunk_start_row arguments instead of the full page_state_s* struct.
  • Regression test (parquet_chunked_reader_test.cu): Adds TestChunkedReadWithPlainListOfStringSpanningPages, covering full reads, chunked reads at multiple byte limits, and skip_rows/num_rows windows that land on 0-row pages — particularly effective under compute-sanitizer memcheck.

Confidence Score: 5/5

Safe to merge — this is a clean, well-scoped backport of an already-reviewed upstream fix with a comprehensive regression test.

The patch faithfully reproduces the upstream fix with no deviations: function signatures are correctly refactored, all call sites in the five changed CUDA files are updated consistently, and the new page_has_rows_to_process helper is logically equivalent to the old scattered checks while also handling the previously uncovered 0-row list-continuation-page case. The regression test covers the full read, chunked reads at multiple byte limits, and targeted skip_rows/num_rows windows that exercise exactly the boundary condition that triggered the OOB write.

No files require special attention.

Important Files Changed

Filename Overview
patches/fix_parquet_preprocess_zero_row_string_pages.patch New patch file backporting upstream fix for 0-row list page OOB write; logic is correct, signatures are properly refactored, and the helper page_has_rows_to_process correctly handles the list continuation-page edge case.
patches/noop.patch Empty placeholder patch removed now that the real fix patch replaces it.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["compute_page_offset_count\n(host-side buffer sizing)"] --> B{page_has_rows_to_process?}
    B -->|"page.num_rows > 0\n& row range intersects"| C[Include page in buffer size]
    B -->|"page.num_rows == 0\n& !has_repetition"| D[Skip page → return 0]
    B -->|"page.num_rows == 0\n& has_repetition\n(0-row list continuation page)"| E{is_bounds_page OR\nis_page_contained?}
    E -->|Yes| C
    E -->|No| D
    C --> F["GPU decode kernels\nwrite offsets to allocated buffer ✓"]
    D --> G["GPU decode kernels\nstay within allocated buffer ✓"]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["compute_page_offset_count\n(host-side buffer sizing)"] --> B{page_has_rows_to_process?}
    B -->|"page.num_rows > 0\n& row range intersects"| C[Include page in buffer size]
    B -->|"page.num_rows == 0\n& !has_repetition"| D[Skip page → return 0]
    B -->|"page.num_rows == 0\n& has_repetition\n(0-row list continuation page)"| E{is_bounds_page OR\nis_page_contained?}
    E -->|Yes| C
    E -->|No| D
    C --> F["GPU decode kernels\nwrite offsets to allocated buffer ✓"]
    D --> G["GPU decode kernels\nstay within allocated buffer ✓"]
Loading

Reviews (2): Last reviewed commit: "delete noop patch" | Re-trigger Greptile

Signed-off-by: Paul Mattione <pmattione@nvidia.com>
@pmattione-nvidia
pmattione-nvidia marked this pull request as ready for review July 15, 2026 22:36
@ttnghia

ttnghia commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Can we hot fix in cudf instead?

@pmattione-nvidia

Copy link
Copy Markdown
Collaborator Author

Can we hot fix in cudf instead?

No they aren't interested in it.

diff --git a/cpp/tests/io/parquet_chunked_reader_test.cu b/cpp/tests/io/parquet_chunked_reader_test.cu
index 0b4910ef4ee..c131a9983e0 100644
--- a/cpp/tests/io/parquet_chunked_reader_test.cu
+++ b/cpp/tests/io/parquet_chunked_reader_test.cu

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think tests are needed here, as we never run them. However, it's good to keep the redundant tests here to reproduce if needed.

@ttnghia

ttnghia commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Please do not rebase this into main/26.08 release but fix in cudf instead.

@pmattione-nvidia

Copy link
Copy Markdown
Collaborator Author

Please do not rebase this into main/26.08 release but fix in cudf instead.

It's already fixed in cudf 26.08, as per the above PR link. this pr is just for a patch to 26.06 release only.

@NvTimLiu

Copy link
Copy Markdown
Collaborator

build

1 similar comment
@pmattione-nvidia

Copy link
Copy Markdown
Collaborator Author

build

@pmattione-nvidia
pmattione-nvidia merged commit 0b3969c into NVIDIA:release/26.06 Jul 16, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants