laskoviymishka commented on code in PR #1444:
URL: https://github.com/apache/iceberg-go/pull/1444#discussion_r3580566566
##########
table/metadata.go:
##########
@@ -1622,6 +1651,163 @@ func (c *commonMetadata) EncryptionKeys()
iter.Seq[EncryptionKey] {
return slices.Values(c.EncryptionKeyList)
}
+func cloneSchema(schema *iceberg.Schema) *iceberg.Schema {
+ if schema == nil {
+ return nil
+ }
+
+ return iceberg.NewSchemaWithIdentifiers(
Review Comment:
Worth a thought on the hot path: cloning through `NewSchemaWithIdentifiers`
runs `init()`, so every clone comes back with cold lazy caches (`idToName`,
`nameToID`, etc.). `CurrentSchema()`/`Schemas()` get hit per-file during scan
planning, so this is a fair bit of re-derivation and allocation on large
tables. (Also `schema.Fields()` already returns a `slices.Clone`, so
`cloneNestedFields` clones the top-level slice a second time.)
Not blocking, but I'd consider either caching the clone or shallow-cloning
while sharing the atomic cache pointers and copying only the mutable exported
fields (`ID`, `IdentifierFieldIDs`). wdyt?
##########
table/metadata_getters_test.go:
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "encoding/json"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/require"
+)
+
+func TestMetadataGettersReturnDefensiveCopies(t *testing.T) {
+ parentID := int64(1)
+ refMinSnapshots := 2
+ refMaxSnapshotAge := int64(3)
+ refMaxAge := int64(4)
+ metadata := commonMetadata{
+ CurrentSchemaID: 1,
+ DefaultSpecID: 1,
+ SchemaList:
[]*iceberg.Schema{iceberg.NewSchemaWithIdentifiers(1, []int{1},
iceberg.NestedField{
+ ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64,
Required: true,
+ })},
+ Specs: []iceberg.PartitionSpec{iceberg.NewPartitionSpecID(1,
iceberg.PartitionField{
+ SourceIDs: []int{1}, FieldID: 1000, Name: "id",
Transform: iceberg.IdentityTransform{},
+ })},
+ SnapshotList: []Snapshot{{
+ SnapshotID: 2,
+ ParentSnapshotID: &parentID,
+ Summary: &Summary{
+ Operation: OpAppend,
+ Properties: map[string]string{"source": "test"},
+ },
+ }},
+ CurrentSnapshotID: &[]int64{2}[0],
+ SnapshotRefs: map[string]SnapshotRef{
+ MainBranch: {
+ SnapshotID: 2,
+ SnapshotRefType: BranchRef,
+ MinSnapshotsToKeep: &refMinSnapshots,
+ MaxSnapshotAgeMs: &refMaxSnapshotAge,
+ MaxRefAgeMs: &refMaxAge,
+ },
+ },
+ Props: map[string]string{"owner": "iceberg"},
+ SortOrderList: []SortOrder{{
+ orderID: 1,
+ fields: []SortField{{
+ SourceIDs: []int{10},
+ Transform: iceberg.IdentityTransform{},
+ }},
+ }},
+ }
+
+ original, err := json.Marshal(metadata)
+ require.NoError(t, err)
+
+ properties := metadata.Properties()
+ properties["owner"] = "mutated"
+
+ schemas := metadata.Schemas()
+ schemas[0].ID = 99
+ schemas[0].IdentifierFieldIDs[0] = 99
+ nestedFields := schemas[0].Fields()
+ nestedFields[0].Name = "mutated"
+ nestedFields[0].Type = iceberg.PrimitiveTypes.String
+
+ partitionSpecs := metadata.PartitionSpecs()
+ partitionField := partitionSpecs[0].Field(0)
+ partitionField.SourceIDs[0] = 99
Review Comment:
This block doesn't actually guard the `SourceIDs` clone.
`PartitionField.SourceIDs` is `json:"-"` (partitions.go:47), so the closing
`JSONEq` never sees it change — and `Field(0)` returns a value copy, so
`partitionField.Name = "mutated"` is a no-op too. The whole assertion would
still pass if we deleted `slices.Clone(fields[i].SourceIDs)` from the clone
helper. Same holds for the `PartitionSpec()` / `PartitionSpecByID()` blocks
just below.
I'd assert the returned `SourceIDs` is a distinct backing slice directly —
mutate it, then read `metadata.Specs[0].Field(0).SourceIDs` back (the test is
in package `table`) and require it unchanged — rather than relying on the JSON
round-trip.
##########
table/metadata_getters_test.go:
##########
@@ -0,0 +1,126 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package table
+
+import (
+ "encoding/json"
+ "testing"
+
+ "github.com/apache/iceberg-go"
+ "github.com/stretchr/testify/require"
+)
+
+func TestMetadataGettersReturnDefensiveCopies(t *testing.T) {
+ parentID := int64(1)
+ refMinSnapshots := 2
+ refMaxSnapshotAge := int64(3)
+ refMaxAge := int64(4)
+ metadata := commonMetadata{
+ CurrentSchemaID: 1,
+ DefaultSpecID: 1,
+ SchemaList:
[]*iceberg.Schema{iceberg.NewSchemaWithIdentifiers(1, []int{1},
iceberg.NestedField{
+ ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64,
Required: true,
+ })},
+ Specs: []iceberg.PartitionSpec{iceberg.NewPartitionSpecID(1,
iceberg.PartitionField{
+ SourceIDs: []int{1}, FieldID: 1000, Name: "id",
Transform: iceberg.IdentityTransform{},
+ })},
+ SnapshotList: []Snapshot{{
+ SnapshotID: 2,
+ ParentSnapshotID: &parentID,
+ Summary: &Summary{
+ Operation: OpAppend,
+ Properties: map[string]string{"source": "test"},
+ },
+ }},
+ CurrentSnapshotID: &[]int64{2}[0],
+ SnapshotRefs: map[string]SnapshotRef{
+ MainBranch: {
+ SnapshotID: 2,
+ SnapshotRefType: BranchRef,
+ MinSnapshotsToKeep: &refMinSnapshots,
+ MaxSnapshotAgeMs: &refMaxSnapshotAge,
+ MaxRefAgeMs: &refMaxAge,
+ },
+ },
+ Props: map[string]string{"owner": "iceberg"},
+ SortOrderList: []SortOrder{{
+ orderID: 1,
+ fields: []SortField{{
+ SourceIDs: []int{10},
+ Transform: iceberg.IdentityTransform{},
+ }},
+ }},
+ }
+
+ original, err := json.Marshal(metadata)
+ require.NoError(t, err)
+
+ properties := metadata.Properties()
+ properties["owner"] = "mutated"
+
+ schemas := metadata.Schemas()
+ schemas[0].ID = 99
+ schemas[0].IdentifierFieldIDs[0] = 99
+ nestedFields := schemas[0].Fields()
+ nestedFields[0].Name = "mutated"
+ nestedFields[0].Type = iceberg.PrimitiveTypes.String
+
+ partitionSpecs := metadata.PartitionSpecs()
+ partitionField := partitionSpecs[0].Field(0)
+ partitionField.SourceIDs[0] = 99
+ partitionField.Name = "mutated"
+
+ currentSchema := metadata.CurrentSchema()
+ currentSchema.ID = 100
+ defaultSpec := metadata.PartitionSpec()
+ defaultSpecField := defaultSpec.Field(0)
+ defaultSpecField.SourceIDs[0] = 100
+ byIDSpec := metadata.PartitionSpecByID(1)
+ require.NotNil(t, byIDSpec)
+ byIDField := byIDSpec.Field(0)
+ byIDField.SourceIDs[0] = 101
+
+ snapshots := metadata.Snapshots()
+ snapshots[0].ParentSnapshotID = new(int64)
+ snapshots[0].Summary.Properties["source"] = "mutated"
+
+ byID := metadata.SnapshotByID(2)
+ require.NotNil(t, byID)
+ *byID.ParentSnapshotID = 99
+ byID.Summary.Properties["source"] = "mutated"
+
+ current := metadata.CurrentSnapshot()
+ require.NotNil(t, current)
+ current.Summary.Properties["source"] = "mutated"
+
+ refs := metadata.Ref()
+ *refs.MinSnapshotsToKeep = 99
+ for _, ref := range metadata.Refs() {
+ *ref.MaxSnapshotAgeMs = 99
+ }
+
+ sortOrders := metadata.SortOrders()
+ fields := sortOrders[0].Fields()
+ for _, field := range fields {
+ field.SourceIDs[0] = 99
Review Comment:
Same dead-coverage issue here — `Fields()` yields `SortField` by value, so
`field.SourceIDs[0] = 99` mutates a loop-local copy, and `SortField.SourceIDs`
is `json:"-"` on top of that. Nothing in this block fails if `cloneSortOrder`
is removed. Since we're in package `table`, I'd reach into
`sortOrders[0].fields` directly and mutate a serializable field
(`Direction`/`NullOrder`), or compare the slice identity against the original.
##########
table/metadata.go:
##########
@@ -1622,6 +1651,163 @@ func (c *commonMetadata) EncryptionKeys()
iter.Seq[EncryptionKey] {
return slices.Values(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 {
+ 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)
Review Comment:
I think there's still a shallow copy hiding here. We deep-copy `Type` but
leave `InitialDefault` and `WriteDefault` as shared `any` values — and those
can hold a `[]byte` (BinaryLiteral/FixedLiteral), so a caller can do
`schema.Fields()[0].WriteDefault.([]byte)[0] = 0xFF` and reach right back into
the original.
This looks like the class zeroshade was pointing at in the shallow-copy
thread. I'd add a small `cloneDefault(v any) any` that `slices.Clone`s the
`[]byte`-backed cases and passes everything else through, then apply it to both
defaults here.
##########
table/metadata.go:
##########
@@ -1622,6 +1651,163 @@ func (c *commonMetadata) EncryptionKeys()
iter.Seq[EncryptionKey] {
return slices.Values(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 {
+ clones := make([]*iceberg.Schema, len(schemas))
Review Comment:
One behavior change to watch: `make([]T, len(x))` turns a nil `SchemaList`
into a non-nil empty slice, so `Schemas()` now returns `[]` where it used to
return `nil` (and marshals to `[]` instead of `null`). Same for
`clonePartitionSpecs`/`cloneSortOrders`/`cloneSnapshots`. Any caller using `==
nil` to mean "unset", or comparing JSON round-trips, will shift. I'd guard each
with `if x == nil { return nil }`.
##########
table/metadata.go:
##########
@@ -1591,23 +1617,26 @@ 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)
Review Comment:
While we're clamping down here, the getters just below still hand out shared
internal state — `Statistics()`, `PartitionStatistics()` and `EncryptionKeys()`
yield value copies of the top-level structs, but
`BlobMetadata.Properties`/`Fields` and `EncryptionKey.Properties` are still the
internal maps/slices. `metadata.Statistics()...BlobMetadata[0].Properties["x"]
= "y"` mutates the real `StatisticsList`.
Same class as what this PR fixes, on the same interface. Either deep-clone
these too, or explicitly document which getters guarantee copies and defer the
rest — either's fine, but I'd rather not ship an interface where half the
getters are safe and half aren't, silently.
##########
table/metadata.go:
##########
@@ -1622,6 +1651,163 @@ func (c *commonMetadata) EncryptionKeys()
iter.Seq[EncryptionKey] {
return slices.Values(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 {
+ 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)
+ }
+
+ return clones
+}
+
+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)
+ fields[i].SourceIDs = slices.Clone(fields[i].SourceIDs)
Review Comment:
Minor: this `slices.Clone` is redundant — `NewPartitionSpecID` already
clones `SourceIDs` when it builds the field (partitions.go:372), so we clone
twice. Can drop the explicit one here.
--
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]
