You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "twuebi (via GitHub)" <gi...@apache.org> on 2026/04/28 13:18:59 UTC
[PR] fix(parquet): align dictionary fallback with parquet-mr [arrow-go]
twuebi opened a new pull request, #786:
URL: https://github.com/apache/arrow-go/pull/786
### 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:
- 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.
### 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): align dictionary fallback with parquet-mr [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #786:
URL: https://github.com/apache/arrow-go/pull/786
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): align dictionary fallback with parquet-mr [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #786:
URL: https://github.com/apache/arrow-go/pull/786#discussion_r3162525230
##########
parquet/file/column_writer.go:
##########
@@ -502,6 +542,9 @@ func (w *columnWriter) WriteDictionaryPage() error {
page := NewDictionaryPage(buffer, int32(dictEncoder.NumEntries()), w.props.DictionaryPageEncoding())
written, err := w.pager.WriteDictionaryPage(page)
w.totalBytesWritten += written
+ if err == nil {
+ w.dictPageWritten = true
+ }
Review Comment:
```suggestion
w.dictPageWritten = err == nil
```
##########
parquet/file/column_writer.go:
##########
@@ -427,12 +462,17 @@ func (w *columnWriter) FlushBufferedDataPages() (err error) {
return err
}
}
+ return w.drainBufferedDataPages()
+}
+// drainBufferedDataPages writes out and releases any pages buffered while
+// dictionary encoding was active. Unlike FlushBufferedDataPages, it does not
+// touch the current encoder's unflushed values, so the caller can re-encode
+// them as PLAIN during a dictionary fallback.
+func (w *columnWriter) drainBufferedDataPages() (err error) {
for i, p := range w.pages {
defer p.Release()
if err = w.WriteDataPage(p); err != nil {
- // To keep pages in consistent state,
- // remove the pages that will be released using above defer call.
Review Comment:
why remove the comment?
##########
parquet/file/column_writer.go:
##########
@@ -264,14 +269,44 @@ func (w *columnWriter) commitWriteAndCheckPageLimit(numLevels, numValues int64)
w.numBufferedValues += numLevels
w.numDataValues += numValues
- enc := w.currentEncoder.EstimatedDataEncodedSize()
- if enc >= w.props.DataPageSize() {
+ // While dictionary encoding is active we size pages by raw input bytes
+ // instead of the RLE-indices' encoded size. Mirrors parquet-mr's
+ // FallbackValuesWriter.getBufferedSize — it keeps dict pages roughly
+ // the same raw-byte footprint as the PLAIN pages they'd otherwise be,
+ // which also pulls the first-page compression check into the same
+ // cadence parquet-mr uses and avoids committing dict pages that only
+ // look cheap because their RLE indices are tiny.
+ var bufferedSize int64
+ if w.hasDict && !w.fallbackToNonDict {
+ bufferedSize = w.currentEncoder.(encoding.DictEncoder).ObservedRawSize()
+ } else {
+ bufferedSize = w.currentEncoder.EstimatedDataEncodedSize()
+ }
+ if bufferedSize >= w.props.DataPageSize() {
return w.FlushCurrentPage()
}
return nil
}
func (w *columnWriter) FlushCurrentPage() error {
+ // Before committing what would be the first dict-encoded data page,
+ // check whether dictionary encoding is actually saving space against
+ // a PLAIN baseline. This mirrors parquet-mr's
+ // FallbackValuesWriter.getBytes + isCompressionSatisfying: if the
+ // dictionary plus the encoded indices meet or exceed the raw input
+ // bytes, fall back to PLAIN now and discard the dictionary — avoiding
+ // the mid-cardinality case where a dict page stays in the file
+ // alongside PLAIN pages without any net compression win.
+ if w.hasDict && !w.fallbackToNonDict && !w.dictPageWritten && len(w.pages) == 0 && w.fallbackFn != nil {
+ dictEnc := w.currentEncoder.(encoding.DictEncoder)
+ rawSize := dictEnc.ObservedRawSize()
+ encodedSize := dictEnc.EstimatedDataEncodedSize()
+ dictSize := int64(dictEnc.DictEncodedSize())
+ if rawSize > 0 && dictSize+encodedSize >= rawSize {
Review Comment:
do we actually need the rawSize > 0 check?
##########
parquet/internal/encoding/memo_table.go:
##########
@@ -307,6 +311,10 @@ func (m *binaryMemoTableImpl) CopyOffsets(out []int32) {
m.CopyOffsetsSubset(0, out)
}
+func (m *binaryMemoTableImpl) Value(i int) []byte {
+ return m.builder.Value(i)
+}
Review 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.
##########
parquet/file/column_writer.go:
##########
@@ -140,6 +140,11 @@ type columnWriter struct {
totalCompressedBytes int64
closed bool
fallbackToNonDict bool
+ dictPageWritten bool
+ // 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()
Review 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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org