fix(parquet): align dictionary fallback with parquet-mr#786
Merged
zeroshade merged 5 commits intoapache:mainfrom Apr 30, 2026
Merged
fix(parquet): align dictionary fallback with parquet-mr#786zeroshade merged 5 commits intoapache:mainfrom
zeroshade merged 5 commits intoapache:mainfrom
Conversation
On dictionary overflow, arrow-go always flushed the dictionary page
and any buffered dict-encoded data pages before switching to PLAIN,
even when no dict-encoded data page had been cut. On mid-cardinality
columns the result was a 4-encoding chunk layout
(PLAIN_DICTIONARY, PLAIN, RLE, PLAIN) that bloated output by 20-30%
versus parquet-mr.
Mirror parquet-mr's FallbackValuesWriter:
- Discard the dictionary and re-encode buffered indices as PLAIN
when no dict-encoded data page has been flushed yet; only emit
the dictionary page once a dict-encoded page is committed.
- Before the first dict-encoded page, fall back to PLAIN if
dict + indices >= raw input bytes.
- Size dict-encoded pages by raw input bytes (not the RLE indices'
encoded size) so the page cadence matches PLAIN.
Adds DictEncoder.FallBackTo / ObservedRawSize and exposes
BinaryMemoTable.Value for the fallback translation.
zeroshade
reviewed
Apr 29, 2026
| rawSize := dictEnc.ObservedRawSize() | ||
| encodedSize := dictEnc.EstimatedDataEncodedSize() | ||
| dictSize := int64(dictEnc.DictEncodedSize()) | ||
| if rawSize > 0 && dictSize+encodedSize >= rawSize { |
Member
There was a problem hiding this comment.
do we actually need the rawSize > 0 check?
Comment on lines
-434
to
-435
| // To keep pages in consistent state, | ||
| // remove the pages that will be released using above defer call. |
Comment on lines
+545
to
+547
| if err == nil { | ||
| w.dictPageWritten = true | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| if err == nil { | |
| w.dictPageWritten = true | |
| } | |
| w.dictPageWritten = err == nil |
Comment on lines
+144
to
+147
| // fallbackFn is set by each typed column writer at construction to its | ||
| // own FallbackToPlain. It lets the base FlushCurrentPage trigger | ||
| // fallback without needing to know the concrete value type. | ||
| fallbackFn func() |
Member
There was a problem hiding this comment.
FallbackToPlain is already part of the ColumnChunkWriter interface, could we just modify logic in checkDictionarySizeLimit etc. instead of needing to pass the function callback like this?
Comment on lines
+314
to
+316
| func (m *binaryMemoTableImpl) Value(i int) []byte { | ||
| return m.builder.Value(i) | ||
| } |
Member
There was a problem hiding this comment.
this is the legacy map-based implementation. Luckily this function already exists in internal/hashing/xxh3_memo_table.go for the binary memo table that is actually being used.
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
2 participants
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
On dictionary overflow, arrow-go always flushed the dictionary page and any buffered dict-encoded data pages before switching to PLAIN, even when no dict-encoded data page had been cut. On mid-cardinality columns the result was a 4-encoding chunk layout (PLAIN_DICTIONARY, PLAIN, RLE, PLAIN) that bloated output by 20-30% versus parquet-mr.
This was noticed when testing iceberg-go's recently added compaction feature, where some tables with particular high cardinality columns would see a 30% size increase after compaction.
What changes are included in this PR?
Mirror parquet-mr's FallbackValuesWriter:
Adds DictEncoder.FallBackTo / ObservedRawSize and exposes BinaryMemoTable.Value for the fallback translation.
Are these changes tested?
Yes, as part of the PR and also e2e while testing compaction in iceberg-go.
Are there any user-facing changes?
No public API changes, only observable thing should be the dropped double encoding.