NopeCharon opened a new pull request, #3879: URL: https://github.com/apache/parquet-java/pull/3879
### Rationale for this change Some old writers recorded a `total_compressed_size` that leaves out the dictionary page header, so the column chunk continues past its recorded end. `ParquetFileReader` reads the last chunk of each row group as a `WorkaroundChunk`, which is meant to fetch the missing bytes from the file. That workaround is broken in three ways, so these files can't be read: 1. **Truncated page data.** `readAsBytesInput` reads the missing bytes into a new buffer with `f.readFully(lastBuffer)` but never flips it. The buffer is passed on with `position == limit`, so it adds no bytes and the page comes back short. Reported in #2926. 2. **Truncated page header.** The header fallback overrides `readPageHeader()`, but since the encryption merge (PARQUET-1884) `readAllPages` calls `readPageHeader(BlockCipher.Decryptor, byte[])`. The override is never called, so a chunk that ends inside its last page header fails with `can not read class org.apache.parquet.format.PageHeader: Socket is closed by peer`. 3. **Vectored reads.** The workaround assumes the file stream is positioned at the end of the chunk. Vectored reads, which are on by default, don't move the stream, so the missing bytes come from the wrong offset, for example `EOFException: Reached the end of stream with 5 bytes left to read`. ### What changes are included in this PR? All changes are in `ParquetFileReader.WorkaroundChunk`: - Flip the buffer that holds the missing bytes before returning it. - Override `readPageHeader(BlockCipher.Decryptor, byte[])` instead of the no-argument method, and pass the decryptor and AAD to both header reads. - Before the first read of the missing bytes, seek the file stream to the end of the chunk (`fileOffset + size`). With sequential reads the stream is already there. With vectored reads the seek is required. The fallback still runs only on the last chunk of a read and only when the recorded size is too short. Files with correct sizes take the same path as before. ### Are these changes tested? Yes. `TestReadChunkWithShortCompressedSize` writes a dictionary-encoded file and rewrites its footer with a shorter `total_compressed_size` for the column chunk, as the old writers did. It covers: - the chunk ending 13 bytes early, inside the last page's data - the chunk ending 2 bytes into the last page's header Each case runs with sequential and with vectored reads. The vectored case uses a stream that serves vectored reads like Hadoop's positioned reads, which leave the stream position unchanged. The test asserts that the vectored path actually ran and that every value reads back correctly. All 4 cases fail without this change: one wrong-data assertion, one `EOFException`, and two page header read errors. With the change they pass. Removing only the new seek makes both vectored cases fail again. Locally on JDK 17 with Thrift 0.25.0, `spotless:check` passes and the full `parquet-hadoop` unit test suite passes: 907 tests, 0 failures, 26 skipped. I skipped the JUnit 4 import check for this run because master currently fails it in `TestFooterIntegrity`. That is unrelated to this change and fixed separately. ### Are there any user-facing changes? Yes: files whose last column chunk under-reports its compressed size can now be read. Nothing changes for files with correct sizes. Closes #2926 -- 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]
