Skip to content

GH-47981: [C++][Parquet] Add compatibility with non-compliant RLE stream - #47992

Merged
pitrou merged 4 commits into
apache:mainfrom
pitrou:gh47981-pq-impala-file
Nov 13, 2025
Merged

pitrou merged 4 commits into
apache:mainfrom
pitrou:gh47981-pq-impala-file

Conversation

@pitrou

@pitrou pitrou commented Oct 29, 2025 •

Copy link
Copy Markdown
Member

Rationale for this change

RLE-bit-packed streams are required by the Parquet spec to have 8-padded bit-packed runs, but some non-compliant encoders (such as Polars versions before pola-rs/polars#13883) might generate a truncated last bit-packed run, which nevertheless contains enough logical values.

What changes are included in this PR?

  1. Compatibility code for non-compliant RLE streams as described above
  2. Guard against zero-size dictionaries to avoid hitting an assertion in DictionaryConverter

Are these changes tested?

Yes, by additional unit tests.

Are there any user-facing changes?

No, except a bugfix.

@pitrou

pitrou commented Oct 29, 2025

Copy link
Copy Markdown
Member Author

@AntoinePrv FYI

@pitrou
pitrou force-pushed the gh47981-pq-impala-file branch from b3eadff to b362c3a Compare November 10, 2025 08:13
@pitrou
pitrou marked this pull request as ready for review November 10, 2025 09:37
@pitrou
pitrou requested a review from wgtmac November 10, 2025 09:38
@pitrou

pitrou commented Nov 10, 2025

Copy link
Copy Markdown
Member Author

@AntoinePrv Are you available to review this?

@pitrou
pitrou force-pushed the gh47981-pq-impala-file branch from fcb3c55 to f613124 Compare November 10, 2025 10:06
@pitrou

pitrou commented Nov 10, 2025

Copy link
Copy Markdown
Member Author

@ursabot please benchmark lang=C++

@voltrondatabot

Copy link
Copy Markdown

Benchmark runs are scheduled for commit f613124. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

@pitrou
pitrou force-pushed the gh47981-pq-impala-file branch from f613124 to ba8cd20 Compare November 10, 2025 13:16
@pitrou

pitrou commented Nov 10, 2025

Copy link
Copy Markdown
Member Author

@ursabot please benchmark lang=C++

@voltrondatabot

Copy link
Copy Markdown

Benchmark runs are scheduled for commit ba8cd20. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

@conbench-apache-arrow

Copy link
Copy Markdown

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit f613124.

There were 9 benchmark results indicating a performance regression:

The full Conbench report has more details.

@conbench-apache-arrow

Copy link
Copy Markdown

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit ba8cd20.

There were 6 benchmark results indicating a performance regression:

The full Conbench report has more details.

@pitrou
pitrou force-pushed the gh47981-pq-impala-file branch from ba8cd20 to 959327f Compare November 12, 2025 09:01
@pitrou

pitrou commented Nov 12, 2025

Copy link
Copy Markdown
Member Author

@ursabot please benchmark lang=C++

@voltrondatabot

Copy link
Copy Markdown

Benchmark runs are scheduled for commit 959327f. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

@pitrou

pitrou commented Nov 12, 2025

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit -g cpp

@pitrou
pitrou requested a review from adamreeve November 12, 2025 09:37
@github-actions

Copy link
Copy Markdown

Revision: 959327f

Submitted crossbow builds: ursacomputing/crossbow @ actions-f55022a5e6

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-42-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@conbench-apache-arrow

Copy link
Copy Markdown

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 959327f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I just have a minor question.

// Bit-packed run
constexpr auto kMaxCount = bit_util::CeilDiv(internal::max_size_for_v<rle_size_t>, 8);
if (ARROW_PREDICT_FALSE(count == 0 || count > kMaxCount)) {
if (ARROW_PREDICT_FALSE(count == 0 || count >= kMaxCount)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because kMaxCount uses CeilDiv and is therefore one large than the actual max.

That said, this is a bit confusing so I could try to change to kMaxCount = internal::max_size_for_v<rle_size_t> / 8.

@pitrou
pitrou force-pushed the gh47981-pq-impala-file branch from 959327f to 5fe9995 Compare November 13, 2025 08:47
@pitrou

pitrou commented Nov 13, 2025

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit -g cpp

@github-actions

Copy link
Copy Markdown

Revision: 5fe9995

Submitted crossbow builds: ursacomputing/crossbow @ actions-926cb57385

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-42-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 0c20874.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.

Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
…LE stream (apache#47992)

### Rationale for this change

RLE-bit-packed streams are required by the Parquet spec to have 8-padded bit-packed runs, but some non-compliant encoders (such as Polars versions before pola-rs/polars#13883) might generate a truncated last bit-packed run, which nevertheless contains enough *logical* values.

### What changes are included in this PR?

1. Compatibility code for non-compliant RLE streams as described above
2. Guard against zero-size dictionaries to avoid hitting an assertion in `DictionaryConverter`

### Are these changes tested?

Yes, by additional unit tests.

### Are there any user-facing changes?

No, except a bugfix.

* GitHub Issue: apache#47981

Authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
viirya added a commit to apache/spark that referenced this pull request Oct 14, 2026
### What changes were proposed in this pull request?

This is a follow-up of SPARK-59812. It makes `VectorizedRleValuesReader`, which decodes definition/repetition levels, dictionary ids and RLE booleans, fail with a `ParquetDecodingException` when the encoded data ends early or a header is invalid.

- `readNextGroup()` throws when there is no encoded data left, instead of returning false. Callers never read more values than the page declares, so running out of data always means a corrupt page. All the `if (currentCount == 0 && !readNextGroup()) break;` loop guards become `if (currentCount == 0) readNextGroup();`. With a bit width of 0, `initFromPage` already provides every value of the page, so reaching `readNextGroup()` also means reading past the end.
- `readNextGroup()` skips runs of length 0, so `currentCount` is always positive when it returns. The existing group decoding moves unchanged into a `readGroup()` helper that `readNextGroup()` calls in a loop. The Parquet spec does not allow runs of length 0; skipping them keeps what the batch paths already did, so no file that could be read before fails because of them.
- For a bit-packed run, check that `numGroups * 8` does not overflow and that `numGroups * bitWidth` bytes are left before allocating the value buffer.
- `initFromPage` rejects a level length that is cut off, negative or larger than the bytes left in the page, and a dictionary id bit width above 32.
- `VectorizedColumnReader` now also adds the page or dictionary and the column to a `ParquetDecodingException` thrown while initializing a page (in `readPageV1` and `readPageV2`) or decoding the dictionary page, as it already did for an `IOException`, so a corrupt length still reports the column. It wraps it in a new `ParquetDecodingException` that includes the original message, so the exception type stays the same.
- A read error inside a run header or an RLE value (e.g. the data ends in the middle of a header) now also starts with "Corrupted RLE data".
- A dictionary-encoded page with no data at all (not even the bit width byte) no longer decodes every value as id 0. Reading from it fails, like parquet-java's `DictionaryValuesReader` ("Attempt to read from empty page"). An all-null page, which reads no ids, still works.

The added checks run once per run or page, not per value.

### Why are the changes needed?

SPARK-34863 made `readNextGroup()` return false at the end of the stream. Before that, it called `in.read()`, which throws an `EOFException` that was wrapped in a `ParquetDecodingException`. Since then, on a corrupt page:

- The batch read and skip loops stop early, so dictionary ids are only partially read or skipped, and the query returns wrong results without an error.
- For definition and repetition levels, `readBatch` and `readBatchRepeated` return without making progress, and `VectorizedColumnReader.readBatch` loops forever because `rowsToReadInBatch` and `valuesToReadInPage` never change.
- For repeated columns, when only the definition levels end early, the missing definition levels are silently left as 0, which produces wrong nested values.
- `readBooleans` ignores the return value, and loops forever at the end of the stream.
- `readInteger` ignores the return value and decrements `currentCount` below 0, returning the last run's value again. It also returns the value of a run of length 0 instead of skipping it.
- A bit-packed header with a large group count overflows `numGroups * 8` and `numGroups * bitWidth` (the latter was added by SPARK-56895). `in.slice` with a negative length moves a `SingleBufferInputStream` backwards, and a large count allocates up to an `int[2^31 - 8]` buffer before noticing that the bytes are missing.
- In Parquet 1.18.1, `SingleBufferInputStream.sliceBuffers` only rejects `length > remaining`, so a negative level length moves the stream backwards and the next section of the page is decoded from the wrong offset.

When the encoded data ends early, the non-vectorized reader already fails: parquet-java's `RunLengthBitPackingHybridDecoder` throws "Reading past RLE/BitPacking stream.".

This PR does not change how a last bit-packed run that is shorter than its padding is handled. Some non-compliant writers produce it (e.g. Polars before pola-rs/polars#13883). The vectorized reader already rejected it before this change, because `in.slice` threw an `EOFException`, while parquet-java and Arrow C++ (apache/arrow#47992) accept it when the bytes cover the values that are read. Accepting it is left for a follow-up, SPARK-59853.

A bit-packed run whose bytes are all present is still not bounded by the number of values in the page, so a corrupt header can still make the reader allocate a buffer much larger than the page needs. Rejecting such runs would also fail non-compliant pages that decode correctly today (e.g. 9 values written as a run of 3 groups), so bounding them is left for a follow-up.

### Does this PR introduce _any_ user-facing change?

Only for corrupt or non-compliant Parquet files: reading a page whose levels, dictionary ids or RLE booleans end early, or have an invalid length, bit width or bit-packed header, now fails with a `ParquetDecodingException` instead of returning wrong values, spinning forever, or allocating a buffer for a bit-packed run whose bytes are missing. Valid files are not affected. With `spark.sql.files.ignoreCorruptFiles=true`, the rest of such a file is now skipped.

A few of these pages could be read before and give the right result by chance, and now fail:
- Dictionary ids (or RLE boolean values) that end early, when every row after the cut is skipped by column index filtering, e.g. the rest of the page before a row range in a later page. The skip stopped silently at the end of the data. This was possible since 3.3.0 (SPARK-34863); 3.2 failed the same way as this PR.
- A dictionary-encoded page whose data is empty (no bit width byte, which the spec requires), when all its non-null rows are skipped, or when the dictionary has a single entry, so that decoding every id as 0 happened to be right. This was the behavior since the vectorized reader was added in 2.0.0.

Selecting rows after the cut, or a dictionary with more than one entry, returns wrong values for the same pages without this PR. With `spark.sql.files.ignoreCorruptFiles=true`, such a file is no longer read in full: from the batch that reaches such a page, the rest of the file, including later row groups, is skipped with only a warning in the log.

### How was this patch tested?

New unit tests in `VectorizedRleValuesReaderSuite`. The ones on hand-built dictionary id, boolean and length sections (9 of 14) run both on a single buffer and on the same data split into 1-byte buffers, so that every part of the data longer than 1 byte, including a slice of it, is read from a `MultiBufferInputStream`. Negative tests check the expected message, and the ones that read values run on a separate thread with a timeout, since some looped forever before this change.
- Truncated dictionary ids on `readIntegers`, `skipIntegers` and `readInteger`; truncated booleans on `readBooleans`; truncated definition levels on `readBatch` (with and without definition level output, and with row ranges that skip past the truncation); truncated repetition levels on `readBatchRepeated`; truncated definition levels on `readBatchRepeated` (with and without row ranges); reading past a page with bit width 0.
- Cut-off, negative and oversized (up to `Int.MaxValue`) level lengths, and dictionary id bit widths above 32.
- Bit-packed runs that are truncated, that overflow `currentCount` to 0 or a negative value, or that would allocate a huge buffer.
- Data ending inside a run header or an RLE value.
- A dictionary-encoded page with no data, and runs of length 0.
- Positive cases: reads and skips that end exactly at the end of the encoded values for mixed RLE and bit-packed dictionary ids and booleans, with a level length that covers exactly the rest of the page.

New end-to-end tests in `ParquetIOSuite` corrupt a written file: a shortened definition level run fails instead of hanging, and an oversized length of the definition levels (V1 page) or of RLE boolean values (V2 page) reports the page and column once. A new test in `ParquetVectorizedSuite` checks that a corrupted dictionary page reports the column.

All the new negative tests fail without this change. Existing suites also pass: `ParquetEncodingSuite`, `ParquetVectorizedSuite`, `ParquetColumnIndexSuite`, `ParquetIOSuite`, `ParquetV1QuerySuite`, `ParquetV2QuerySuite`, `ParquetDeltaByteArrayEncodingSuite`, `ParquetDeltaLengthByteArrayEncodingSuite`, `VectorizedPlainValuesReaderSuite` and `ParquetVectorUpdaterSuite`.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 5.5)

Closes #59117 from viirya/SPARK-59832-rle-truncated.

Authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Liang-Chi Hsieh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants