dongjoon-hyun commented on PR #59118:
URL: https://github.com/apache/spark/pull/59118#issuecomment-5881314653
Thank you for extending this hardening to the BYTE_STREAM_SPLIT reader,
@viirya. The new checks look correct to me. Every single-value read, batch read
and skip is bounded. `total > valueCount - offset` is overflow-safe, unlike
parquet-java's `indexInStream + n > valuesCount`. `initFromPage` now matches
parquet-java 1.18.1's `ByteStreamSplitValuesReader`. A few comments:
1. The count check in `initFromPage` is only an upper bound. A data section
whose length is off by a whole multiple of the value width still passes and is
decoded with the wrong stride. For example:
- A nullable FLOAT page with `num_values=10` and 5 nulls, but 24 bytes of
data, decodes all 5 values from the wrong bytes without any error.
- A required INT32 page of 10,000 values that lost exactly one value
returns two full 4,096-row batches of wrong values before the end-of-page check
fires, so `LIMIT`/`show()` never sees an error.
parquet-java has the same limitation, so this is not a regression.
However, the exact count is known for columns with `maxDefinitionLevel == 0`
(`num_values`) and for `DataPageV2` (`num_values - num_nulls`). Could we check
those cases exactly, or at least narrow the PR description ("instead of ...
silently wrong values") and the code comment to what is guaranteed?
2. L73-76: "every value would be decoded from the wrong bytes" holds for a
short page. With 1..W-1 trailing bytes the stride is still correct, and only
the reads past the last whole value return garbage. The new "extra bytes" test
builds exactly that case.
3. Tests:
- The existing end-to-end BYTE_STREAM_SPLIT tests only write V1 pages
with flat schemas and no filter. The new upper-bound check relies on
`page.getValueCount()` including nulls, and the skip paths only run with
column-index row ranges. Could we add a round trip with `PARQUET_2_0` pages,
nulls and an array column, and/or BYTE_STREAM_SPLIT cases in
`ParquetColumnIndexSuite`?
- "batch reads and skips that end exactly at the end of the page" doesn't
check the values it reads. `readIntegersAsLongs`, `readIntegersAsDoubles`,
`readFloatsAsDoubles`, `readLongsAsInts`, `readBytes` and `readShorts` of this
reader have no value-level test anywhere, so asserting them here would be cheap.
- To pin the overflow safety, maybe `skipBatch(reader, 1)` followed by
`op(reader, Int.MaxValue, cv)`. A rewrite to `offset + total > valueCount`
passes all the current tests.
- `assertCorrupted` could use `withClue(msg)` and an implicit `Position`.
Otherwise, when an op throws nothing or throws an
`ArrayIndexOutOfBoundsException`, the failure doesn't say which op in the loop
failed.
- The FLBA test duplicates `newReader`/`assertCorrupted` and covers only
part of the negative matrix (empty page, short page, nulls, negative total are
missing). A width-5 FLBA subclass of the abstract suite would inherit all of it.
4. Nit: `nextIndex()` is equivalent to `checkRemaining(1); return
offset++;`. Also, a negative `total` can only come from a caller bug, not from
the file, so reporting it as "Corrupted BYTE_STREAM_SPLIT page" may be
misleading.
Since this reader (SPARK-56894) is already in `branch-4.3` and
[v4.3.0-rc1](https://github.com/apache/spark/releases/tag/v4.3.0-rc1)
(2026-08-31), could we merge this to `branch-4.x` and `branch-4.3` as well?
`branch-4.2`/`branch-4.1` don't need it because they don't have this reader.
--
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]
