dongjoon-hyun commented on PR #59087:
URL: https://github.com/apache/spark/pull/59087#issuecomment-5861546534
Thank you for working on this, @viirya. The negative-length and short-skip
handling looks correct to me. I checked that every `skipValues` implementation
passes the right width for its physical type, so valid pages never over-skip,
and that the new negative tests fail without this change.
A few suggestions:
1. `readLength()` / `checkLength()` only reject negative lengths. Could we
also reject `len > in.available()`?
- `readGeoData` in both readers uses `in.readNBytes(len)`, which returns
a short array at the end of the page instead of failing. An oversized length is
silently accepted for the last value of a page (or surfaces as
`WKB_PARSE_ERROR`), while `readBinary` / `skipBinary` fail with
`ParquetDecodingException` on the same bytes.
- On a `MultiBufferInputStream` (e.g. uncompressed pages when Hadoop
vectored IO is not used), `slice(len)` allocates `len` bytes before it detects
EOF.
2. `VectorizedDeltaLengthByteArrayReader` does not check that the rows it
consumes were decoded. `lengthsVector` is sized by `valueCount` but filled with
`getTotalValueCount()` values, so undecoded rows read a zero length (silently
empty values), and a larger header count throws
`ArrayIndexOutOfBoundsException` in `initFromPage`. Since all lengths are
decoded up front, validating them once in `initFromPage` (count, sign, and the
sum over `[0, count)` against `available()`) plus a per-call `currentRow +
total <= count` bound may be simpler than four per-use `checkLength` calls. A
follow-up is fine too.
3. The negative-length checks added to the two `readGeoData` paths are not
covered by the new tests.
4. Nits:
- `checkLength` reports "DELTA_LENGTH_BYTE_ARRAY page" also for
DELTA_BYTE_ARRAY suffixes.
- `skipBooleans` could use the new `skipFully` helper (its message lacks
the byte count).
- The DELTA_LENGTH read paths still drop the cause.
- "moves the stream position backwards" only holds for
`SingleBufferInputStream`; `MultiBufferInputStream` treats `n <= 0` as a no-op.
--
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]
