zeroshade commented on code in PR #2002:
URL: https://github.com/apache/iceberg-go/pull/2002#discussion_r4064583094
##########
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:
Correcting myself here: I wrote this without reading the earlier rounds. The
walk exists because @laskoviymishka asked for it in round 1
([discussion_r4008128730](https://github.com/apache/iceberg-go/pull/2002#discussion_r4008128730)),
precisely to avoid materializing a whole `parquet.Schema` per writer init.
Withdrawing the "just always use `dictCostFallbackViaParquet`" ask — that call
was already made, with a perf rationale for wide schemas and short-lived
microbatch writers that I did not weigh.
Two more corrections to what I wrote above: `RunEndEncodedType` *is*
unwrapped here (added in response to
[discussion_r4044956123](https://github.com/apache/iceberg-go/pull/2002#discussion_r4044956123)),
and the list/map branch and `DictionaryType` gaps were both closed in earlier
rounds too. I framed settled ground as new.
What I would still like is much narrower.
`TestDictCostFallbackWalkMatchesToParquet` covers flat / nested / variant /
decimal / dict, but not `RunEndEncodedType` — which the walk explicitly unwraps
— or `arrow.Null`. Since a wrong leaf path fails silently here (the property
matches no column, no error, no panic, and the zstd size regression this PR
fixes quietly returns), that lock-step test is the only thing standing between
the walk and a silent regression. Two more cases in the table would close it.
For the record, not as an argument to change the design now: the walk has
taken three rounds of leaf-naming fixes (list/map coverage, `DictionaryType`,
`RunEndEncodedType`). That is a reason to keep that test table exhaustive as
arrow-go grows types, rather than a reason to revisit the approach.
##########
table/arrow_utils.go:
##########
@@ -436,7 +436,7 @@ func (c convertToIceberg) Primitive(dt arrow.DataType)
(result iceberg.NestedFie
switch dt.ExtensionName() {
case "arrow.uuid":
result.Type = iceberg.PrimitiveTypes.UUID
- case "parquet.variant":
+ case extensions.VariantExtensionName,
extensions.LegacyVariantExtensionName:
Review Comment:
@nssalian apologies for the delay, answering the direct question.
Release notes work for me. There is no `CHANGELOG.md` in the repo and I do
not want this PR to be the one that invents one, so the upgrade note belongs in
the GitHub release for the version carrying this bump. Consider that unblocked
— please go ahead with the remaining items.
One thing I do want to reopen before this lands. @laskoviymishka floated
keeping the legacy name emitted from `VisitVariant` for one release and then
self-dismissed it, and I do not think that dismissal got tested. The reason I
would weigh it: the old-reader failure here is not a graceful degrade, it is
the `default:` panic in the extension-name switch. A release note only helps
operators who read it *before* upgrading; anyone running a mixed-version fleet
who does not gets a crash in the reader rather than a variant column quietly
read as a struct.
So the question is how much we care about the rolling-upgrade window. If the
answer is that iceberg-go is pre-1.0, variant support is new, and nobody has a
mixed 18.7/18.8 fleet reading variant columns in production yet, then release
notes alone are fine and I am happy to sign off on that basis. If we think
there are real readers out there, emitting the legacy name for one release
costs us very little and turns a panic into a non-event.
I do not have strong evidence either way on variant adoption.
@laskoviymishka, you raised it originally — what is your read? I am fine
landing as-is if you both think the window is theoretical.
--
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]
