zeroshade commented on code in PR #1444:
URL: https://github.com/apache/iceberg-go/pull/1444#discussion_r3647331945


##########
table/metadata.go:
##########
@@ -1709,35 +1735,283 @@ func (c *commonMetadata) CurrentSnapshot() *Snapshot {
        return c.SnapshotByID(*c.CurrentSnapshotID)
 }
 
-func (c *commonMetadata) SortOrders() []SortOrder { return c.SortOrderList }
+func (c *commonMetadata) SortOrders() []SortOrder {
+       return cloneSortOrders(c.SortOrderList)
+}
+
 func (c *commonMetadata) SortOrder() SortOrder {
        for _, s := range c.SortOrderList {
                if s.OrderID() == c.DefaultSortOrderID {
-                       return s
+                       return cloneSortOrder(s)
                }
        }
 
-       return UnsortedSortOrder
+       return cloneSortOrder(UnsortedSortOrder)
 }
 
 func (c *commonMetadata) DefaultSortOrder() int {
        return c.DefaultSortOrderID
 }
 
 func (c *commonMetadata) Properties() iceberg.Properties {
-       return c.Props
+       return maps.Clone(c.Props)
 }
 
 func (c *commonMetadata) Statistics() iter.Seq[StatisticsFile] {
-       return slices.Values(c.StatisticsList)
+       return slices.Values(cloneStatisticsFiles(c.StatisticsList))
 }
 
 func (c *commonMetadata) PartitionStatistics() 
iter.Seq[PartitionStatisticsFile] {
-       return slices.Values(c.PartitionStatsList)
+       return slices.Values(slices.Clone(c.PartitionStatsList))
 }
 
 func (c *commonMetadata) EncryptionKeys() iter.Seq[EncryptionKey] {
-       return slices.Values(c.EncryptionKeyList)
+       return slices.Values(cloneEncryptionKeys(c.EncryptionKeyList))
+}
+
+func cloneSchema(schema *iceberg.Schema) *iceberg.Schema {
+       if schema == nil {
+               return nil
+       }
+
+       return iceberg.NewSchemaWithIdentifiers(

Review Comment:
   Non-blocking (perf): cloning via `NewSchemaWithIdentifiers` runs `init()`, 
so every `CurrentSchema()`/`Schemas()` call returns cold `idToName`/`nameToID` 
caches. These are hit per-file during scan planning — worth a benchmark or a 
cache-preserving clone before merge.



##########
table/metadata.go:
##########
@@ -1709,35 +1735,283 @@ func (c *commonMetadata) CurrentSnapshot() *Snapshot {
        return c.SnapshotByID(*c.CurrentSnapshotID)
 }
 
-func (c *commonMetadata) SortOrders() []SortOrder { return c.SortOrderList }
+func (c *commonMetadata) SortOrders() []SortOrder {
+       return cloneSortOrders(c.SortOrderList)
+}
+
 func (c *commonMetadata) SortOrder() SortOrder {
        for _, s := range c.SortOrderList {
                if s.OrderID() == c.DefaultSortOrderID {
-                       return s
+                       return cloneSortOrder(s)
                }
        }
 
-       return UnsortedSortOrder
+       return cloneSortOrder(UnsortedSortOrder)
 }
 
 func (c *commonMetadata) DefaultSortOrder() int {
        return c.DefaultSortOrderID
 }
 
 func (c *commonMetadata) Properties() iceberg.Properties {
-       return c.Props
+       return maps.Clone(c.Props)
 }
 
 func (c *commonMetadata) Statistics() iter.Seq[StatisticsFile] {
-       return slices.Values(c.StatisticsList)
+       return slices.Values(cloneStatisticsFiles(c.StatisticsList))
 }
 
 func (c *commonMetadata) PartitionStatistics() 
iter.Seq[PartitionStatisticsFile] {
-       return slices.Values(c.PartitionStatsList)
+       return slices.Values(slices.Clone(c.PartitionStatsList))
 }
 
 func (c *commonMetadata) EncryptionKeys() iter.Seq[EncryptionKey] {
-       return slices.Values(c.EncryptionKeyList)
+       return slices.Values(cloneEncryptionKeys(c.EncryptionKeyList))
+}
+
+func cloneSchema(schema *iceberg.Schema) *iceberg.Schema {
+       if schema == nil {
+               return nil
+       }
+
+       return iceberg.NewSchemaWithIdentifiers(
+               schema.ID,
+               slices.Clone(schema.IdentifierFieldIDs),
+               cloneNestedFields(schema.Fields())...,
+       )
+}
+
+func cloneSchemas(schemas []*iceberg.Schema) []*iceberg.Schema {
+       if schemas == nil {
+               return nil
+       }
+
+       clones := make([]*iceberg.Schema, len(schemas))
+       for i, schema := range schemas {
+               clones[i] = cloneSchema(schema)
+       }
+
+       return clones
+}
+
+func cloneNestedFields(fields []iceberg.NestedField) []iceberg.NestedField {
+       clones := slices.Clone(fields)
+       for i := range clones {
+               clones[i].Type = cloneSchemaType(clones[i].Type)
+               clones[i].InitialDefault = 
cloneDefault(clones[i].InitialDefault)
+               clones[i].WriteDefault = cloneDefault(clones[i].WriteDefault)
+       }
+
+       return clones
+}
+
+func cloneDefault(value any) any {
+       switch value := value.(type) {
+       case []byte:
+               return slices.Clone(value)
+       case iceberg.BinaryLiteral:
+               return iceberg.BinaryLiteral(slices.Clone([]byte(value)))
+       case iceberg.FixedLiteral:
+               return iceberg.FixedLiteral(slices.Clone([]byte(value)))
+       case []any:
+               clones := make([]any, len(value))
+               for i, item := range value {
+                       clones[i] = cloneDefault(item)
+               }
+
+               return clones
+       case map[string]any:
+               clones := make(map[string]any, len(value))
+               for key, item := range value {
+                       clones[key] = cloneDefault(item)
+               }
+
+               return clones
+       default:
+               return value
+       }
+}
+
+func cloneSchemaType(typ iceberg.Type) iceberg.Type {
+       switch typ := typ.(type) {
+       case *iceberg.StructType:
+               return &iceberg.StructType{FieldList: 
cloneNestedFields(typ.FieldList)}
+       case *iceberg.ListType:
+               return &iceberg.ListType{
+                       ElementID:       typ.ElementID,
+                       Element:         cloneSchemaType(typ.Element),
+                       ElementRequired: typ.ElementRequired,
+               }
+       case *iceberg.MapType:
+               return &iceberg.MapType{
+                       KeyID:         typ.KeyID,
+                       KeyType:       cloneSchemaType(typ.KeyType),
+                       ValueID:       typ.ValueID,
+                       ValueType:     cloneSchemaType(typ.ValueType),
+                       ValueRequired: typ.ValueRequired,
+               }
+       default:
+               return typ
+       }
+}
+
+func clonePartitionSpec(spec iceberg.PartitionSpec) iceberg.PartitionSpec {
+       fields := make([]iceberg.PartitionField, spec.NumFields())
+       for i := range fields {
+               fields[i] = spec.Field(i)

Review Comment:
   Minor: this `slices.Clone(SourceIDs)` is redundant — `NewPartitionSpecID` 
already clones `SourceIDs` when building the field.



-- 
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]

Reply via email to