tanmayrauth commented on code in PR #1415:
URL: https://github.com/apache/iceberg-go/pull/1415#discussion_r3525969084
##########
schema.go:
##########
@@ -310,6 +310,44 @@ func (s *Schema) MarshalJSON() ([]byte, error) {
}{Type: "struct", Fields: s.fields, Alias: &aliasCopy})
}
+func cloneFields(fields []NestedField) []NestedField {
+ if len(fields) == 0 {
+ return nil
+ }
+
+ cloned := make([]NestedField, len(fields))
+ for i, field := range fields {
+ cloned[i] = field
+ cloned[i].Type = cloneType(field.Type)
+ }
+
+ return cloned
+}
+
+func cloneType(t Type) Type {
Review Comment:
`func cloneType(t Type) Type {`
The default branch returns t unchanged, so leaf/parameterized types
(Decimal, Fixed, Geometry, Geography, Variant, ...) stay aliased — only the
container types are deep-copied. I think that's intentional since the package
treats those as immutable, but a one-line comment saying so would keep the
partial clone from reading as a missed case.
##########
schema.go:
##########
@@ -257,7 +257,7 @@ func (s *Schema) Type() string { return "struct" }
// AsStruct returns a Struct with the same fields as the schema which can
// then be used as a Type.
-func (s *Schema) AsStruct() StructType { return StructType{FieldList:
s.fields} }
+func (s *Schema) AsStruct() StructType { return StructType{FieldList:
cloneFields(s.fields)} }
Review Comment:
`func (s *Schema) AsStruct() StructType { return StructType{FieldList:
cloneFields(s.fields)} }`
This method isn't only a public convenience — it's called on internal hot
paths that never mutate the result: the schema visitors (Visit at
schema.go:599, PreOrderVisit at schema.go:770, visitStructWithPartner at
schema.go:1485) and the metrics evaluator constructors
(table/evaluators.go:713, :728, :1247). Those evaluators are built
per-manifest/per-file during scan planning (table/scanner.go,
arrow_scanner.go:955), and the visitors run on every projection/prune/promote
pass. All of them just read FieldList.
With this change every one of them now recursively clones the whole schema
tree on each call, which is a meaningful regression for wide/deeply-nested
schemas during scans. Could we keep an internal non-cloning path (callers use
s.fields directly, or a private asStructRef()) and only clone in the exported
AsStruct()? That preserves the public-safety guarantee without taxing the
read-only internal callers.
##########
schema_test.go:
##########
@@ -219,6 +219,39 @@ func TestNestedFieldToString(t *testing.T) {
}
}
+func TestSchemaAsStructClonesTopLevelFieldList(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "foo", Type:
iceberg.PrimitiveTypes.String, Required: true},
+ )
+
+ structType := schema.AsStruct()
+ structType.FieldList[0].Name = "hijacked"
+
+ assert.Equal(t, "foo", schema.Field(0).Name)
+}
+
+func TestSchemaAsStructClonesNestedTypes(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},
+ }},
Review Comment:
Nice coverage on the struct case. Since cloneType also special-cases
*ListType and *MapType (schema.go:337, :342), could you add a nested list and
map aliasing test too? Otherwise a future tweak to those branches could regress
without a failing test.
--
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]
