Skip to content

GH-3336: Read dictionary pages of old parquet-mr files with default settings - #3875

Open
sankalpsthakur wants to merge 1 commit into
apache:masterfrom
sankalpsthakur:GH-3336-old-writer-dictionary-chunks
Open

sankalpsthakur wants to merge 1 commit into
apache:masterfrom
sankalpsthakur:GH-3336-old-writer-dictionary-chunks

Conversation

@sankalpsthakur

Copy link
Copy Markdown
Contributor

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) uses WorkaroundChunk only for the very last chunk that was read, and WorkaroundChunk.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 name or only comment_col does not fail but returns wrong values, e.g. row 8 reads ALGERIA instead of INDIA.

What changes are included in this PR?

  • Every chunk that ends a consecutive part list becomes a WorkaroundChunk that 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.
  • The bytes read that way are flipped before they are used.
  • The completion is limited to 100 bytes, the same bound the C++ reader uses for this bug, so a really truncated chunk still ends in an EOFException.

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 TestOldWriterDictionaryChunks reads 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, for name only and for comment_col only, 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.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant