fix(schema): prevent AsStruct aliasing#1415
Conversation
tanmayrauth
left a comment
tanmayrauth
left a comment
There was a problem hiding this comment.
Fix is correct and the tests nail the aliasing. My one real concern is that AsStruct is on the scan/visitor hot paths and none of those callers mutate — so this makes them all deep-clone for nothing. Details inline.
| // 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)} } |
There was a problem hiding this 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.
| return cloned | ||
| } | ||
|
|
||
| func cloneType(t Type) Type { |
There was a problem hiding this 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.
| Name: "person", | ||
| Type: &iceberg.StructType{FieldList: []iceberg.NestedField{ | ||
| {ID: 2, Name: "name", Type: iceberg.PrimitiveTypes.String}, | ||
| }}, |
There was a problem hiding this 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.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The aliasing fix is right in principle, but the current placement is a scan-path performance regression (inline). Also missing *ListType/*MapType aliasing tests — the added tests only cover the top-level field list and nested *StructType.
| // 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)} } | ||
| func (s *Schema) NumFields() int { return len(s.fields) } |
There was a problem hiding this comment.
AsStruct() is on the read-only scan critical path (called by schema visitors and evaluator constructors that never mutate the result). Deep-cloning the full field tree on every call is a real regression for wide/nested schemas. Suggest a shared/private asStructRef() for internal read-only callers, keeping the deep clone only in the exported AsStruct().
| cloned.ValueType = cloneType(typed.ValueType) | ||
|
|
||
| return &cloned | ||
| default: |
There was a problem hiding this comment.
The default branch returns t unchanged (primitive/Decimal/Fixed/Geometry/Geography/Variant) — likely intentional since these are immutable values, but a one-line comment would keep it from reading as a missed case.
1710eca to
1425820
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest commit — the hot-path concern is fully resolved:
AsStruct()still deep-clones for the public/safe path, but the internal read-only visitors now use the new non-cloningasStructRef(), and the evaluator constructors no longer callAsStruct()at all — so scan planning no longer pays for a full field-tree clone.- The
cloneTypedefault branch now has an explanatory comment. - Dedicated
*ListType/*MapTypealiasing tests were added.
LGTM — thanks for the quick turnaround!
3 participants
What changed
Schema.AsStruct()return a deep-clonedStructTypefield list instead of exposing internal schema state.This ensures callers cannot mutate schema internals through
Schema.AsStruct().