zeroshade commented on code in PR #2029:
URL: https://github.com/apache/iceberg-go/pull/2029#discussion_r4065555175
##########
data_file_codec.go:
##########
@@ -333,3 +334,82 @@ func manifestEntrySchemaFor(spec PartitionSpec, schema
*Schema, version int) (*a
return entry.schema, entry.maps, nil
}
+
+func partitionSchemaFingerprint(spec PartitionSpec, schema *Schema) (string,
error) {
+ var key strings.Builder
+ for _, field := range spec.fields {
+ sourceType := Type(UnknownType{})
+ // ResultType only inspects the source type here; borrow it to
avoid
+ // cloning nested source types on every schema-cache hit.
+ if sourceField, ok := schema.FindFieldByIDRef(field.SourceID(),
internal.SchemaRef{}); ok {
+ sourceType = sourceField.Type
+ }
+ resultType := field.Transform.ResultType(sourceType)
Review Comment:
Worth recording the tradeoff this borrow makes, though I don't think it
should hold the PR.
`Transform` is a public interface, and `sourceType` here is storage owned by
the caller's `Schema` rather than a copy. A caller-defined `ResultType` that
mutates a nested `StructType`, `ListType`, or `MapType` therefore corrupts the
caller's schema, and concurrent codec calls can race on that shared storage.
The old `PartitionType` path took a defensive clone via `FindTypeByID`, so this
narrows a guarantee that previously held. I confirmed the mechanism with an
embedded `IdentityTransform` whose `ResultType` renames a nested child:
`manifestEntrySchemaFor` succeeds, and a later schema lookup returns the
corrupted name.
Mutating an argument you don't own is a caller bug, and every built-in
transform returns a fresh type, so in practice this is fine. If you want it
airtight without giving up the fast path, fast-path the known built-ins and
clone before invoking unknown implementations. Your call — I'm noting it mainly
so the borrow stays a deliberate, documented decision rather than something
rediscovered 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]
