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