Repository navigation
Conversation
vinodkc
force-pushed
the
parquet-plain-then-dict-repro
branch
from
October 22, 2026 20:23
fe4bfa1 to
f646659
Compare
HyukjinKwon
reviewed
Oct 22, 2026
HyukjinKwon
left a comment
Member
There was a problem hiding this comment.
Thanks, I went through it. The approach looks right to me, and both changes go after the actual root causes:
startOffset == 0is the condition from before SPARK-34859 (a5c8866), and it matches the PLAIN branch, which already usesvalueOffset != 0to mean "this batch already has values".startRowId == pageFirstRowIndexis always true on the first read of a page, so it never guarded the PLAIN -> dictionary case.- Reserving from
state.valueOffsetis correct because that's where the values are written. The dictionary ids vector'selementsAppendeddoesn't count values from PLAIN pages.
Please keep both in one PR. I ran the new tests with each fix reverted:
- with neither: all 5 fail; "more values than rows" throws the same
ArrayIndexOutOfBoundsExceptionas in the PR description (arraycopy: last destination index 15 out of bounds for int[10]) - with only the reserve fix: all 5 return wrong values, so the exception turns into silently wrong results
- with only
startOffset == 0: the two multi-value nested tests still hit theArrayIndexOutOfBoundsException
With both, the whole ParquetVectorizedSuite passes.
Next: please file a JIRA (Bug, SQL, correctness label), put the ID in the PR title and the new test names, and drop WIP. branch-3.5/4.0/4.1/4.2/4.x all have both lines, so this should be backported. It cherry-picks cleanly to branch-4.x, while 4.2 and older need small manual backports (context drift from SPARK-59854).
… followed by a dictionary page in a batch
vinodkc
force-pushed
the
parquet-plain-then-dict-repro
branch
from
October 22, 2026 22:56
f646659 to
bbd4e14
Compare
vinodkc
marked this pull request as ready for review
October 22, 2026 22:58
Collaborator
|
The AI review requested by @HyukjinKwon did not finish. A committer can add the |
This branch has not been deployed
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.
What changes were proposed in this pull request?
This PR fixes two bugs in the vectorized Parquet reader that are hit when, within one batch, a dictionary-encoded data page follows a PLAIN-encoded data page of the same column chunk.
Wrong results from lazy dictionary decoding (
VectorizedColumnReader.readBatch). After reading a dictionary-encoded page, the reader attaches the dictionary to the column vector (lazy decoding) only if no non-dictionary values were added to the batch yet. Since SPARK-34859 this was checked withstartRowId == pageFirstRowIndex, butParquetReadState.resetForNewPagesetsrowIdtopageFirstRowIndex, so the check is always true on the first read of a page. The dictionary was then attached over values that a PLAIN page had already written to the batch, and those rows were decoded from dictionary id slots that were never written. This PR restores the check tostartOffset == 0(no values in the batch yet), so such pages take the existing eagerdecodeDictionaryIdspath. This affects flat and nested columns.startOffset == 0was the check before SPARK-34859, and it matches that PR's intent: when asked in review whyreadState.rowId == pageFirstRowIndexis a condition for lazy decoding, the answer was that it is "just to check whether we are at the first iteration of the loop". Rows skipped by column index filtering are not written to the batch, so they don't changestartOffset.ArrayIndexOutOfBoundsExceptionfor repeated columns (VectorizedRleValuesReader.readValues). Values are written atstate.valueOffset, butvalues/nullswere reserved withreserveAdditional(total), i.e. relative to their ownelementsAppended. For a dictionary-encoded page,valuesis the dictionary ids vector, whoseelementsAppendeddoes not count the values a PLAIN page already wrote to the batch, so the reservation falls short andputIntsoverflows. This PR reservesstate.valueOffset + totalinstead. In all the other cases the two are equal, so the behavior there is unchanged.With off-heap column vectors (
spark.sql.columnVector.offheap.enabled=true),OffHeapColumnVector.putIntsdoes not check bounds, so this overflow writes past the allocated native memory instead of throwing.Why are the changes needed?
The Parquet format allows a column chunk to mix dictionary-encoded and PLAIN data pages in any order. parquet-mr only falls back from dictionary to PLAIN for good, so files written by Spark do not hit this, but other writers do. We hit this with files written by Snowflake Openflow, where a list element column of a shredded variant has the pages
RLE_DICTIONARY -> PLAIN -> RLE_DICTIONARY. Reading such files:sum(aggregate(tags, 0L, (a, x) -> a + x))gives31228268instead of31232468, orjava.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 30504 out of bounds for int[3100]atOnHeapColumnVector.putIntsviaVectorizedRleValuesReader.readIntegersRepeated.Setting
spark.sql.parquet.enableNestedColumnVectorizedReader=falseavoids it only for scans that read nested columns; scans of flat columns stay vectorized and can still return wrong results.The wrong results (1.) exist since Spark 3.2.0 (SPARK-34859). The overflow (2.) can only happen with the nested column vectorized reader, so since Spark 3.3.0.
Does this PR introduce any user-facing change?
Yes, it is a bug fix. Reading Parquet files in which a dictionary-encoded page follows a PLAIN page in the same batch now returns correct results instead of wrong values or an
ArrayIndexOutOfBoundsException.How was this patch tested?
Added tests to
ParquetVectorizedSuitethat build column chunks with a given sequence of PLAIN and dictionary-encoded pages and read them with batch sizes that span the pages, with on-heap and off-heap vectors:ArrayIndexOutOfBoundsExceptionbefore the fix)checkAnswerin the suite now also asserts that the reader returns exactly the expected number of rows; before, a reader that stopped early would pass.Also ran
ParquetVectorizedSuite,ParquetEncodingSuite,ParquetIOSuite,ParquetColumnIndexSuite,ParquetTypeWideningSuite,ParquetV1QuerySuite,ParquetV2QuerySuite,ParquetFileFormatV1Suite,ParquetFileFormatV2Suite,ParquetV1SchemaPruningSuiteandParquetV2SchemaPruningSuite.Was this patch authored or co-authored using generative AI tooling?
Yes, Generated-by: Claude Code (Claude Opus 5.5)