zeroshade commented on code in PR #2002:
URL: https://github.com/apache/iceberg-go/pull/2002#discussion_r4064549594
##########
table/internal/parquet_files.go:
##########
@@ -711,9 +711,103 @@ func getWriteProperties(writeProps any, arrowSchema
*arrow.Schema) (*parquet.Wri
wp = append(wp, parquet.WithStoreDecimalAsInteger(true))
}
+ // Match Iceberg Java: apply parquet-mr's cost-based dictionary
fallback to every leaf
+ // column so high-cardinality columns fall back to PLAIN rather than
keeping a dictionary.
+ // arrow-go otherwise enables it only for uncompressed columns, so zstd
(our default) would
+ // retain dictionaries on all-distinct columns and roughly double their
size.
+ costFallback, err := dictCostFallbackProps(arrowSchema, wp)
+ if err != nil {
+ return nil, err
+ }
+ wp = append(wp, costFallback...)
+
return parquet.NewWriterProperties(wp...), nil
}
+// dictCostFallbackProps returns a WithDictionaryCostFallbackFor(true)
property per leaf, walking the arrow schema directly (extensions unwrapped) and
falling back to pqarrow.ToParquet for list/map schemas.
+func dictCostFallbackProps(arrowSchema *arrow.Schema, base
[]parquet.WriterProperty) ([]parquet.WriterProperty, error) {
Review Comment:
This walk hand-rolls parquet leaf-path naming for non-list/map schemas, with
`dictCostFallbackViaParquet` as the "authoritative" path only for list/map.
`pqarrow.NewFileWriter` calls `ToParquet` unconditionally (v18.8
`file_writer.go`), and `getWriteProperties` runs immediately before it at line
616 — so this walk avoids only a *second* `ToParquet` call, in exchange for a
permanent obligation to track arrow-go's leaf-naming rules.
The failure mode is what concerns me: a mismatched path means
`WithDictionaryCostFallbackFor` matches no column. No error, no panic — the
zstd size regression this PR exists to fix just silently comes back.
`TestDictCostFallbackWalkMatchesToParquet` locks 5 shapes, but
`RunEndEncodedType` is explicitly unwrapped here yet untested, and `arrow.Null`
is not covered either.
Would you consider always using `dictCostFallbackViaParquet` and dropping
the walk plus `schemaHasListOrMap` (~60 lines and its lock-step test)? If the
extra `ToParquet` per writer measurably matters, that is worth stating in the
comment — otherwise the simplification looks like a clear win.
##########
table/variant_residual.go:
##########
@@ -91,18 +201,144 @@ func buildExtractColumn(col iceberg.VariantExtractColumn,
rec arrow.RecordBatch,
}
if aerr := appendExtractLiteral(bldr, lit); aerr != nil {
- return nil, arrow.Field{}, aerr
+ return nil, aerr
}
}
- field := arrow.Field{
- Name: col.Name,
- Type: dt,
- Nullable: true,
- Metadata: arrow.NewMetadata([]string{ArrowParquetFieldIDKey},
[]string{strconv.Itoa(col.FieldID)}),
+ return bldr.NewArray(), nil
+}
+
+// tryShreddedTypedColumn returns the field's typed leaf column when it is
shredded to exactly dt, else nil.
+func tryShreddedTypedColumn(varr *extensions.VariantArray, path
variant.VariantPath, dt arrow.DataType, mem memory.Allocator) arrow.Array {
+ if path.Len() == 0 || varr.Data().Offset() != 0 {
+ return nil
+ }
+ tv := varr.Shredded()
+ if tv == nil || rootResidualHidesRows(varr, tv) {
+ return nil
+ }
+ n := varr.Len()
+
+ var mask *memory.Buffer
+ shouldBail := false
+ mergeValidity := func(arr arrow.Array) {
Review Comment:
Nit, non-blocking: `mergeValidity` reads `n` bits from each array's validity
buffer without asserting `arr.Len() >= n`, and the zero-copy return at lines
298-301 does not check `cur.Len() == n`.
Both are safe with well-formed Arrow, and arrow's record validation only
rejects *short* columns (`arr.Len() < rows`), so an over-long child would be
silently truncated to the right rows anyway. A cheap `cur.Len() != n -> bail()`
would make the invariant explicit rather than inherited from the producer.
##########
table/internal/parquet_files.go:
##########
@@ -711,9 +711,103 @@ func getWriteProperties(writeProps any, arrowSchema
*arrow.Schema) (*parquet.Wri
wp = append(wp, parquet.WithStoreDecimalAsInteger(true))
}
+ // Match Iceberg Java: apply parquet-mr's cost-based dictionary
fallback to every leaf
+ // column so high-cardinality columns fall back to PLAIN rather than
keeping a dictionary.
+ // arrow-go otherwise enables it only for uncompressed columns, so zstd
(our default) would
+ // retain dictionaries on all-distinct columns and roughly double their
size.
+ costFallback, err := dictCostFallbackProps(arrowSchema, wp)
+ if err != nil {
+ return nil, err
+ }
+ wp = append(wp, costFallback...)
+
return parquet.NewWriterProperties(wp...), nil
}
+// dictCostFallbackProps returns a WithDictionaryCostFallbackFor(true)
property per leaf, walking the arrow schema directly (extensions unwrapped) and
falling back to pqarrow.ToParquet for list/map schemas.
+func dictCostFallbackProps(arrowSchema *arrow.Schema, base
[]parquet.WriterProperty) ([]parquet.WriterProperty, error) {
+ if schemaHasListOrMap(arrowSchema) {
+ return dictCostFallbackViaParquet(arrowSchema, base)
+ }
+
+ var props []parquet.WriterProperty
+ var walk func(prefix string, dt arrow.DataType)
+ walk = func(prefix string, dt arrow.DataType) {
+ if ext, ok := dt.(arrow.ExtensionType); ok {
+ dt = ext.StorageType()
+ }
+ if d, ok := dt.(*arrow.DictionaryType); ok {
+ dt = d.ValueType
+ }
+ if r, ok := dt.(*arrow.RunEndEncodedType); ok {
+ dt = r.Encoded()
+ }
+ if st, ok := dt.(*arrow.StructType); ok {
+ for _, f := range st.Fields() {
+ walk(prefix+"."+f.Name, f.Type)
+ }
+
+ return
+ }
+ props = append(props,
parquet.WithDictionaryCostFallbackFor(prefix, true))
+ }
+ for _, f := range arrowSchema.Fields() {
+ walk(f.Name, f.Type)
+ }
+
+ return props, nil
+}
+
+// dictCostFallbackViaParquet is the authoritative path for list/map schemas,
whose parquet leaf naming the direct walk does not reproduce.
+func dictCostFallbackViaParquet(arrowSchema *arrow.Schema, base
[]parquet.WriterProperty) ([]parquet.WriterProperty, error) {
+ parquetSchema, err := pqarrow.ToParquet(arrowSchema,
parquet.NewWriterProperties(base...), pqarrow.DefaultWriterProps())
Review Comment:
This calls `ToParquet` with `pqarrow.DefaultWriterProps()`, but the actual
writer uses `NewArrowWriterProperties(WithAllocator(mem), WithStoreSchema())`
(line 623).
`storeSchema` does not affect leaf paths today, so this is correct as
written — but the two calls should use the same arrow props so they cannot
drift into disagreeing about column paths later.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
