Skip to content

Fix Parquet V2 inputs to decompression scratch queries - #24006

Open
a-hirota wants to merge 2 commits into
NVIDIA:mainfrom
a-hirota:fix/parquet-v2-decompression-scratch
Open

Fix Parquet V2 inputs to decompression scratch queries#24006
a-hirota wants to merge 2 commits into
NVIDIA:mainfrom
a-hirota:fix/parquet-v2-decompression-scratch

Conversation

@a-hirota

@a-hirota a-hirota commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Description

Chunked Parquet reads with a non-zero input pass limit can hang or encounter an illegal device access on Zstd DataPageV2 files. The extended decompression scratch-size query receives each whole page, including uncompressed repetition/definition levels and pages whose values are not compressed. The decompressor already handles these cases when preparing its actual input buffers.

Use a shared input-selection helper to skip uncompressed V2 pages, advance past level bytes, and exclude pages without a remaining payload. Apply that selection to both cumulative and total decompression-size estimates, so skipped pages cannot dilute the extended/legacy scratch adjustment ratio. Retain full page output sizes as conservative bounds, consistent with the decompressor's codec statistics; prefixes with no compressed pages require no scratch query.

Add 16 reader cases covering V1, V2 with default chunk-level compression, V2 with page-level compression, nullable/list columns, dictionary/plain encoding, mixed compressed/uncompressed pages, and empty pages within Zstd chunks. Each checks values and nulls with small and large pass limits through direct file input and pre-parsed footers. Two direct regression tests check input pointers/lengths and verify that inserting skipped pages cannot reduce the scratch estimate for the same compressed pages.

Keep the existing parquet_read_subrowgroup_chunks benchmark's 144 Snappy/V1 configurations at 512 MiB. Add a separate parquet_read_v2_scratch benchmark with only 12 Zstd/V2 configurations at a committed default of 8 MiB, covering integer/string/list inputs, page-level compression on/off, and pass limits of 0/500,000 bytes.

Closes #24002.

Validation

  • Built libcudf, PARQUET_TEST, HYBRID_SCAN_TEST, and PARQUET_READER_CHUNKS_NVBENCH from main at a8ad2045e17034209e29cec01a6b4a51d87764d7 (26.10), using a separate worktree/build and an isolated dependency environment on NVIDIA GB10 / Linux aarch64: CUDA 13.3, GCC 14.4, RMM/KvikIO 26.10 nightlies, nvCOMP 5.3.0.16.
  • PARQUET_TEST: 566 passed, 1 skipped, 4 disabled. HYBRID_SCAN_TEST: all 97 passed.
  • CUDA 13.3 compute-sanitizer --tool memcheck: all 18 new cases passed, with 0 errors.
  • Negative controls: restoring whole-page input spans makes DecompressionInputs fail on V2 offsets and skipped pages. Restoring unfiltered page-size accounting makes SkippedPagesDoNotReduceScratchEstimate fail: its first compressed-page estimate falls from 3,279 to 1,588 bytes when skipped pages are inserted. These tests detect the defects without relying on nvCOMP hanging on invalid input. Restored the fix and rebuilt before running the full suites and sanitizer above.
  • A local input auditor observed 656 valid Zstd frame prefixes in scratch-query inputs across the 18 cases.
  • Re-ran the issue's PyArrow 21.0.0 / seed=2 / 200,000-row input: all values and nulls matched in 12 runs covering V1/V2, direct/pre-parsed-footer readers, and pass limits of 0, 1,000,000, and 1,024,000,000 bytes; no scratch-query fallback warnings.
  • Benchmark: all 12 new configurations completed with 20 samples each using their committed axis defaults, including the 8 MiB input size. Benchmark registration confirms 144 configurations for the existing sub-row-group benchmark and 12 for the new focused benchmark.
  • Local C++ pre-commit checks passed. The existing Theseus 26.08 code and development environment were not modified.

Checklist

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

@a-hirota
a-hirota requested a review from a team as a code owner September 6, 2026 10:52
@copy-pr-bot

copy-pr-bot Bot commented Sep 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 6, 2026
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review Change Stack

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: 91481361-311a-458d-a36b-cfd8ea105f4b

📥 Commits

Reviewing files that changed from the base of the PR and between 6195c43 and 2fe0ccb.

📒 Files selected for processing (4)
  • cpp/benchmarks/io/parquet/parquet_reader_chunks.cpp
  • cpp/src/io/parquet/reader_impl_chunking_utils.cu
  • cpp/src/io/parquet/reader_impl_chunking_utils.cuh
  • cpp/tests/io/parquet_chunked_reader_test.cu

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved chunked Parquet reading for V2 pages, including nullable and repeated values.
    • Corrected handling of uncompressed, empty, skipped, and compressed page data.
    • Improved reader initialization when checking for additional data or using pre-parsed metadata.
  • Tests

    • Added regression coverage across varied Parquet page configurations and compression states.
  • Benchmarks

    • Expanded Parquet read benchmarks to cover V2 headers, page compression, compression codecs, and selected data types.

Walkthrough

Changes

Parquet chunked-read handling

Layer / File(s) Summary
V2 decompression input selection
cpp/src/io/parquet/reader_impl_chunking_utils.cu, cpp/src/io/parquet/reader_impl_chunking_utils.cuh
Scratch sizing now uses the same compressed value spans as decompression. It skips V2 level bytes, uncompressed pages, and empty payloads.
Chunked-reader regression coverage
cpp/tests/io/parquet_chunked_reader_test.cu
Tests cover decompression input selection, scratch sizing, V2 page variants, skipped pages, metadata validation, and chunked-read equivalence.
Compression and V2 benchmark matrix
cpp/benchmarks/io/parquet/parquet_reader_chunks.cpp
The benchmark uses explicit compression and page-level settings and adds ZSTD V2 scratch cases.

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

Merge Risk: ⚪ Minimal · up to 2fe0c

Chunked Parquet reads now size Zstd decompression scratch space from actual compressed value payloads, avoiding V2 level bytes and skipped pages. The changed behavior is covered across relevant page and reader configurations, with no current merge-blocking risk identified.

Suggested reviewers: vyasr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #24002 by selecting only valid compressed payloads, skipping V2 level bytes and uncompressed pages, preserving conservative scratch estimates, and adding coverage for chunked…
Out of Scope Changes check ✅ Passed The tests, benchmark updates, and exported helper changes support the linked issue and stated PR objectives. No unrelated code changes are evident.
Title check ✅ Passed The title clearly summarizes the main change: correcting Parquet V2 input handling for decompression scratch-size queries.
Description check ✅ Passed The description directly explains the Parquet V2 scratch-size fix, regression tests, benchmarks, validation results, and linked issue.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libcudf Affects libcudf (C++/CUDA) code.

Projects

None yet

1 participant