You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/25 17:12:52 UTC
[PR] fix(schema): protect field getters from mutation [iceberg-go]
fallintoplace opened a new pull request, #1534:
URL: https://github.com/apache/iceberg-go/pull/1534
## Summary
- deep-clone nested types returned by Field and Fields
- return independent values from FindFieldByID and FlatFields
- keep cached schema indexes private from caller mutation
This completes the immutable-schema behavior already used by AsStruct and by the Java and Rust implementations.
## Testing
- go test . -count=1
- all repository packages except io/gocloud
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(schema): protect field getters from mutation [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1534:
URL: https://github.com/apache/iceberg-go/pull/1534#discussion_r3690096424
##########
schema.go:
##########
@@ -323,13 +331,47 @@ func cloneFields(fields []NestedField) []NestedField {
cloned := make([]NestedField, len(fields))
for i, field := range fields {
- cloned[i] = field
- cloned[i].Type = cloneType(field.Type)
+ cloned[i] = cloneField(field)
}
return cloned
}
+func cloneField(field NestedField) NestedField {
Review Comment:
This mutate-a-copy-and-return shape has a small trap: `cloneField(f)` with the result dropped compiles fine and silently skips the clone, which is the exact leak we're closing here. A struct-literal return would make an omitted assignment impossible to write, and let the compiler catch a forgotten field while we're at it. Not blocking — wdyt?
##########
schema.go:
##########
@@ -323,13 +331,47 @@ func cloneFields(fields []NestedField) []NestedField {
cloned := make([]NestedField, len(fields))
for i, field := range fields {
- cloned[i] = field
- cloned[i].Type = cloneType(field.Type)
+ cloned[i] = cloneField(field)
}
return cloned
}
+func cloneField(field NestedField) NestedField {
+ field.Type = cloneType(field.Type)
+ field.InitialDefault = cloneDefault(field.InitialDefault)
+ field.WriteDefault = cloneDefault(field.WriteDefault)
+
+ return field
+}
+
+func cloneDefault(value any) any {
+ switch value := value.(type) {
+ case []byte:
+ return slices.Clone(value)
+ case BinaryLiteral:
+ return BinaryLiteral(slices.Clone([]byte(value)))
+ case FixedLiteral:
+ return FixedLiteral(slices.Clone([]byte(value)))
+ case []any:
+ cloned := make([]any, len(value))
+ for i, item := range value {
+ cloned[i] = cloneDefault(item)
+ }
+
+ return cloned
+ case map[string]any:
+ cloned := make(map[string]any, len(value))
+ for key, item := range value {
+ cloned[key] = cloneDefault(item)
+ }
+
+ return cloned
+ default:
Review Comment:
The default branch is correct today — everything that reaches it (int64, float64, string, bool, uuid.UUID, Decimal) is a value type, so the shallow return can't alias. That's a real invariant though, and it's invisible here. Could we add a short comment listing the expected types and noting each must be a value type or immutable reference? A future default backed by a pointer would otherwise slip through silently.
##########
schema_test.go:
##########
@@ -318,6 +320,87 @@ func TestSchemaAsStructClonesNestedMapTypes(t *testing.T) {
assert.Equal(t, "name", clonedValueType.FieldList[0].Name)
}
+func TestSchemaFieldGettersReturnDefensiveCopies(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ ID: 1,
+ Name: "person",
+ Type: &iceberg.StructType{FieldList: []iceberg.NestedField{
+ {ID: 2, Name: "name", Type: iceberg.PrimitiveTypes.String},
+ }},
+ },
+ iceberg.NestedField{
+ ID: 3,
+ Name: "payload",
+ Type: iceberg.PrimitiveTypes.Binary,
+ InitialDefault: iceberg.BinaryLiteral{1, 2},
+ WriteDefault: map[string]any{"nested": []any{[]byte{3, 4}}},
+ },
+ )
+
+ assertIndependent := func(t *testing.T, personField, payloadField iceberg.NestedField) {
+ t.Helper()
+ person, ok := personField.Type.(*iceberg.StructType)
+ require.True(t, ok)
+ person.FieldList[0].Name = "hijacked"
+ payloadField.InitialDefault.(iceberg.BinaryLiteral)[0] = 9
+ payloadField.WriteDefault.(map[string]any)["nested"].([]any)[0].([]byte)[0] = 9
+
+ actual, ok := schema.FindFieldByID(2)
+ require.True(t, ok)
+ assert.Equal(t, "name", actual.Name)
+ payload, ok := schema.FindFieldByID(3)
+ require.True(t, ok)
+ assert.Equal(t, iceberg.BinaryLiteral{1, 2}, payload.InitialDefault)
+ assert.Equal(t, map[string]any{"nested": []any{[]byte{3, 4}}}, payload.WriteDefault)
+ }
+
+ t.Run("Field", func(t *testing.T) {
+ assertIndependent(t, schema.Field(0), schema.Field(1))
+ })
+ t.Run("Fields", func(t *testing.T) {
+ fields := schema.Fields()
+ assertIndependent(t, fields[0], fields[1])
+ })
+ t.Run("FindFieldByID", func(t *testing.T) {
+ person, ok := schema.FindFieldByID(1)
+ require.True(t, ok)
+ payload, ok := schema.FindFieldByID(3)
+ require.True(t, ok)
+ assertIndependent(t, person, payload)
+ })
+ t.Run("FlatFields", func(t *testing.T) {
+ fields, err := schema.FlatFields()
+ require.NoError(t, err)
+ byID := make(map[int]iceberg.NestedField)
+ for field := range fields {
+ byID[field.ID] = field
+ }
+ assertIndependent(t, byID[1], byID[3])
+ })
Review Comment:
`FindFieldByName`, `FindFieldByNameCaseInsensitive`, and `FindTypeByID` all get their clone transitively by delegating to `FindFieldByID`, so they're correct today. But nothing exercises them directly — a refactor that short-circuits the delegation would still pass this test. Since `assertIndependent` already does the work, a couple more subtests through those finders would lock the contract cheaply. wdyt?
##########
schema.go:
##########
@@ -405,6 +447,18 @@ func (s *Schema) FindFieldByNameCaseInsensitive(name string) (NestedField, bool)
// FindFieldByID is like [*Schema.FindColumnName], but returns the whole
// field rather than just the field name.
func (s *Schema) FindFieldByID(id int) (NestedField, bool) {
+ f, ok := s.FindFieldByIDRef(id, internal.SchemaRef{})
+ if ok {
+ f = cloneField(f)
+ }
+
+ return f, ok
+}
+
+// FindFieldByIDRef returns a schema-owned field without cloning it. It is limited
+// to internal callers that only read the result and need to avoid clone-on-read
+// overhead on hot paths.
+func (s *Schema) FindFieldByIDRef(id int, _ internal.SchemaRef) (NestedField, bool) {
Review Comment:
This is the one surface that deliberately hands back a live reference, so I'd make the contract explicit. The returned `NestedField` is a value copy, but `f.Type` is the same `*StructType`/`*ListType`/`*MapType` pointer the schema owns, so `f.Type.(*StructType).FieldList[0].Name = "x"` reaches straight through and corrupts the live schema.
The two callers today (`FieldPartner`, `castIfNeeded`) are read-only so they're fine, but the doc is the only thing guarding this, and right now it says "limited to internal callers" without saying the Type tree is shared. I'd spell it out: the returned field shares its Type pointer with the schema, and callers must treat `Type` and anything reachable from it as read-only.
While we're here — the `Ref` suffix doesn't really signal "skips clone-on-read" to a reader, but that's a naming taste call; the doc is the important part.
##########
schema_test.go:
##########
@@ -318,6 +320,87 @@ func TestSchemaAsStructClonesNestedMapTypes(t *testing.T) {
assert.Equal(t, "name", clonedValueType.FieldList[0].Name)
}
+func TestSchemaFieldGettersReturnDefensiveCopies(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ ID: 1,
+ Name: "person",
+ Type: &iceberg.StructType{FieldList: []iceberg.NestedField{
+ {ID: 2, Name: "name", Type: iceberg.PrimitiveTypes.String},
+ }},
+ },
+ iceberg.NestedField{
+ ID: 3,
+ Name: "payload",
+ Type: iceberg.PrimitiveTypes.Binary,
+ InitialDefault: iceberg.BinaryLiteral{1, 2},
+ WriteDefault: map[string]any{"nested": []any{[]byte{3, 4}}},
+ },
+ )
+
+ assertIndependent := func(t *testing.T, personField, payloadField iceberg.NestedField) {
+ t.Helper()
+ person, ok := personField.Type.(*iceberg.StructType)
+ require.True(t, ok)
+ person.FieldList[0].Name = "hijacked"
+ payloadField.InitialDefault.(iceberg.BinaryLiteral)[0] = 9
+ payloadField.WriteDefault.(map[string]any)["nested"].([]any)[0].([]byte)[0] = 9
+
+ actual, ok := schema.FindFieldByID(2)
+ require.True(t, ok)
+ assert.Equal(t, "name", actual.Name)
+ payload, ok := schema.FindFieldByID(3)
+ require.True(t, ok)
+ assert.Equal(t, iceberg.BinaryLiteral{1, 2}, payload.InitialDefault)
+ assert.Equal(t, map[string]any{"nested": []any{[]byte{3, 4}}}, payload.WriteDefault)
+ }
+
+ t.Run("Field", func(t *testing.T) {
+ assertIndependent(t, schema.Field(0), schema.Field(1))
+ })
+ t.Run("Fields", func(t *testing.T) {
+ fields := schema.Fields()
+ assertIndependent(t, fields[0], fields[1])
+ })
+ t.Run("FindFieldByID", func(t *testing.T) {
+ person, ok := schema.FindFieldByID(1)
+ require.True(t, ok)
+ payload, ok := schema.FindFieldByID(3)
+ require.True(t, ok)
+ assertIndependent(t, person, payload)
+ })
+ t.Run("FlatFields", func(t *testing.T) {
+ fields, err := schema.FlatFields()
+ require.NoError(t, err)
+ byID := make(map[int]iceberg.NestedField)
+ for field := range fields {
+ byID[field.ID] = field
+ }
+ assertIndependent(t, byID[1], byID[3])
+ })
+}
+
+func TestSchemaFieldRefLookupDoesNotAllocate(t *testing.T) {
+ children := make([]iceberg.NestedField, 50)
+ for i := range children {
+ children[i] = iceberg.NestedField{
+ ID: i + 2, Name: fmt.Sprintf("child_%d", i), Type: iceberg.PrimitiveTypes.String,
+ }
+ }
+ schema := iceberg.NewSchema(0, iceberg.NestedField{
+ ID: 1, Name: "parent", Type: &iceberg.StructType{FieldList: children},
+ })
+ _, ok := schema.FindFieldByIDRef(1, internal.SchemaRef{})
+ require.True(t, ok)
+
+ var field iceberg.NestedField
+ assert.Zero(t, testing.AllocsPerRun(100, func() {
Review Comment:
Two small things on this test. The zero-alloc check leans on `field` being read outside the lambda to stay live — a future escape-analysis change could elide the assignment and pass spuriously; `runtime.KeepAlive(field)` inside the closure pins it. And since this is the one path that intentionally aliases, a companion assertion that a write through the returned ref does reach the schema would document that contract right next to the defensive-copy test. Both optional.
##########
schema.go:
##########
@@ -323,13 +331,47 @@ func cloneFields(fields []NestedField) []NestedField {
cloned := make([]NestedField, len(fields))
for i, field := range fields {
- cloned[i] = field
- cloned[i].Type = cloneType(field.Type)
+ cloned[i] = cloneField(field)
}
return cloned
}
+func cloneField(field NestedField) NestedField {
+ field.Type = cloneType(field.Type)
+ field.InitialDefault = cloneDefault(field.InitialDefault)
+ field.WriteDefault = cloneDefault(field.WriteDefault)
+
+ return field
+}
+
+func cloneDefault(value any) any {
Review Comment:
This is now the third byte-for-byte copy of `cloneDefault` (`table/metadata.go` and `view/metadata.go` have the others). Not asking to fix it in this PR, but could we file a follow-up to hoist `cloneDefault`/`cloneField`/`cloneType` into one place? Right now a new default type has to be handled in three files or it silently aliases in two of them.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(schema): protect field getters from mutation [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1534:
URL: https://github.com/apache/iceberg-go/pull/1534
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org