viirya opened a new pull request, #59117:
URL: https://github.com/apache/spark/pull/59117
### 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.
- 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 negative or larger than the
bytes left in the page.
- 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.
- `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.
The non-vectorized reader already fails on these pages: parquet-java's
`RunLengthBitPackingHybridDecoder` throws "Reading past RLE/BitPacking stream.".
### Does this PR introduce _any_ user-facing change?
Only for corrupt Parquet files: reading a page whose levels, dictionary ids
or RLE booleans end early, or have an invalid length or bit-packed header, now
fails with a `ParquetDecodingException` instead of returning wrong values,
spinning forever or running out of memory. Valid files are not affected.
### How was this patch tested?
New unit tests in `VectorizedRleValuesReaderSuite`. Most run both on a
single buffer and on the same data split across multiple buffers. Tests that
could loop forever before this change run with a timeout.
- 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`; reading past a page with bit width 0.
- Negative and oversized (up to `Int.MaxValue`) level lengths.
- Bit-packed runs that are truncated, that overflow `currentCount` to 0 or a
negative value, or that would allocate a huge buffer.
- 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.
A new end-to-end test in `ParquetIOSuite` shortens the definition level run
of a written file and checks that the vectorized reader fails instead of
hanging.
All the new negative tests fail without this change. Existing suites also
pass: `ParquetEncodingSuite`, `ParquetVectorizedSuite`,
`ParquetColumnIndexSuite`, `ParquetIOSuite`, `ParquetV1QuerySuite` and
`ParquetV2QuerySuite`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
This pull request and its description were written by Isaac.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
