Repository navigation
GH-3336: Read dictionary pages of old parquet-mr files with default settings - #3875
Open
sankalpsthakur wants to merge 1 commit into
Open
sankalpsthakur wants to merge 1 commit into
sankalpsthakur wants to merge 1 commit into
Conversation
…ault settings parquet-mr up to 1.2.8 left the dictionary page header out of the size of the column chunk (PARQUET-816), so the last bytes of such a chunk are not in the buffer that was read for it. ParquetFileReader completes the chunk from the file stream, but only for the last chunk of the row group, and only assuming that the stream is still positioned at the end of that chunk. All part lists are read before the pages are decoded and a vectored read does not move the stream at all, so the read failed with an EOFException or read the wrong bytes. The completed bytes were also never flipped. Every chunk that ends a consecutive part list is now completed from the file at the end offset of its part list, in the sequential and in the vectored path. The completion is limited to the size of a page header.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Files written by parquet-mr up to 1.2.8 do not count the dictionary page header in the size of a column chunk (PARQUET-816), so the end of the last page of such a chunk is not in the buffer that is read for it. ParquetFileReader has a workaround for that, but it only half works.
On master (c972bd0),
ChunkListBuilder.build(ParquetFileReader.java:2059) usesWorkaroundChunkonly for the very last chunk that was read, andWorkaroundChunk.readAsBytesInput(:2361) reads the missing bytes from the file stream at its current position, assuming the stream still sits at the end of that chunk. All part lists are read first and decoded afterwards, and a vectored read never moves the stream, so that assumption does not hold. The bytes that are read are also never flipped, so they are dropped even when the position happens to be right.For nation.dict-malformed.parquet from apache/parquet-testing (the file in #3336) this means: reading all columns fails with an EOFException, and reading only
nameor onlycomment_coldoes not fail but returns wrong values, e.g. row 8 reads ALGERIA instead of INDIA.What changes are included in this PR?
WorkaroundChunkthat knows where its part list ends, in the sequential and in the vectored read. It seeks there before reading the missing bytes instead of trusting the stream position.Healthy files take the same path as before: the workaround only runs where a page would otherwise run past the end of its chunk.
Are these changes tested?
The new
TestOldWriterDictionaryChunksreads a copy of nation.dict-malformed.parquet (2.8 KB, identical to the apache/parquet-testing file) with vectored IO on and off, for all columns, fornameonly and forcomment_colonly, and checks all 25 values of both string columns. The file is in the test resources instead of being downloaded, because of the network problems mentioned in #3670.I ran it on GitHub Actions in my fork with Hadoop 3.3.0 (what the build pins, no vectored IO) and with 3.4.2 (vectored IO): it passes with this change and fails with master's ParquetFileReader on both. With 3.3.0 the whole parquet-hadoop module passes (833 tests, 0 failures; the 24 skipped are TestVectorIoBridge and TestFileRangeBridge, which skip on 3.3.0 because it has no vectored API). Run: https://github.com/sankalpsthakur/parquet-java/actions/runs/39575535093
Are there any user-facing changes?
Files from old parquet-mr writers with dictionary encoded columns can be read with the default settings again. Nothing changes for other files.
This does not cover the second file in the issue, fixed_length_byte_array.parquet. pyarrow cannot read it either ("Unexpected end of stream"), and a comment on the issue points to nulls in a required field (apache/arrow#47662), so it looks like a different problem and the issue stays open for it.
Part of #3336
AI disclosure: Claude Code helped write the fix and the test; I reviewed them and checked the CI results before submitting.