viirya commented on PR #59117: URL: https://github.com/apache/spark/pull/59117#issuecomment-5976913066
@HyukjinKwon Thanks for the re-review. The three test items are addressed in 1a2868d: `interceptCorrupted` checks the expected message, there's a test for data ending inside a run header or an RLE value, and the V2 case is covered end to end. For the last one I moved the catch from `initDataReader` to `readPageV2` (following dongjoon's double-wrapping nit), and the new test checks that the RLE boolean length error in a V2 page reports the page and column once. @dongjoon-hyun Thanks for the thorough check. Addressed in 1a2868d: - The dictionary page catch now also wraps `ParquetDecodingException` (your item 3). - Moved the `ParquetDecodingException` catch from `initDataReader` to `readPageV2`, so V1 and V2 both have one "could not read page" layer. - `interceptCorrupted` runs on a dedicated cached thread pool with `get(30, SECONDS)`, so an `OutOfMemoryError` fails right away, the thread leak warning is gone, and the `scalastyle:off` is no longer needed. I used a cached pool rather than a single thread so that a read stuck in a loop doesn't block the following tests. - Updated the stale `readNextGroup` comments and the class Scaladoc, and switched to `BytesUtils.intToBytes` and `BytesInput.fromUnsignedVarLong`. - Fixed the description: 7 of the 14 unit tests go through `streams`. - On the release notes: I added the cases that could be read before and now fail to the description. Besides the empty dictionary id page, truncated dictionary ids have also read correctly by chance since 3.3.0 when every row after the cut is skipped by column index filtering. For item 1 (an RLE run value wider than the bit width), I tried the per-run check, but a boolean RLE run with value byte `0x03` reads as `true` today, which is likely what the writer meant, and would fail with it. Like the run count cap and the header length limit, I'd rather keep checks that can reject a stream that decodes correctly today out of this PR, and look at them, and item 2, in follow-ups. -- 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]
