tanmayrauth commented on code in PR #1526:
URL: https://github.com/apache/iceberg-go/pull/1526#discussion_r3650522044
##########
udf/metadata.go:
##########
@@ -733,19 +755,24 @@ type metadata struct {
lazyDefinitionsByID func() map[string]*Definition
}
-func (m *metadata) FormatVersion() int { return
m.FormatVersionValue }
-func (m *metadata) FunctionUUID() uuid.UUID { return *m.UUID }
-func (m *metadata) Location() string { return m.Loc }
-func (m *metadata) Definitions() []*Definition { return
m.DefinitionList }
-func (m *metadata) DefinitionLog() []DefinitionLogEntry { return
m.DefinitionLogList }
-func (m *metadata) Properties() iceberg.Properties { return m.Props }
-func (m *metadata) Secure() bool { return m.SecureValue
}
-func (m *metadata) Doc() string { return m.DocValue }
+func (m *metadata) FormatVersion() int { return m.FormatVersionValue }
+func (m *metadata) FunctionUUID() uuid.UUID { return *m.UUID }
+func (m *metadata) Location() string { return m.Loc }
+func (m *metadata) Definitions() []*Definition { return
cloneSlice(m.DefinitionList) }
+func (m *metadata) DefinitionLog() []DefinitionLogEntry {
+ return cloneDefinitionLog(m.DefinitionLogList)
+}
+func (m *metadata) Properties() iceberg.Properties { return
maps.Clone(m.Props) }
+func (m *metadata) Secure() bool { return m.SecureValue }
+func (m *metadata) Doc() string { return m.DocValue }
func (m *metadata) DefinitionByID(definitionID string) (*Definition, bool) {
Review Comment:
This is the getter that actually needed fixing: lazyDefinitionsByID caches
the same *Definition pointers keyed by ID, so a caller mutating
def.DefinitionID on the returned pointer desyncs the map (index keyed by the
old ID, value holding the new one). That real bug is fixable here alone —
return def.Clone() in this method, or stop handing out the indexed pointer —
without making Definitions()/DefinitionLog()/Properties() copy on every call.
Keep this narrow fix and the sweeping copy-on-read in the other getters is
solving a problem the convention already covers.
##########
udf/metadata.go:
##########
@@ -733,19 +755,24 @@ type metadata struct {
lazyDefinitionsByID func() map[string]*Definition
}
-func (m *metadata) FormatVersion() int { return
m.FormatVersionValue }
-func (m *metadata) FunctionUUID() uuid.UUID { return *m.UUID }
-func (m *metadata) Location() string { return m.Loc }
-func (m *metadata) Definitions() []*Definition { return
m.DefinitionList }
-func (m *metadata) DefinitionLog() []DefinitionLogEntry { return
m.DefinitionLogList }
-func (m *metadata) Properties() iceberg.Properties { return m.Props }
-func (m *metadata) Secure() bool { return m.SecureValue
}
-func (m *metadata) Doc() string { return m.DocValue }
+func (m *metadata) FormatVersion() int { return m.FormatVersionValue }
+func (m *metadata) FunctionUUID() uuid.UUID { return *m.UUID }
+func (m *metadata) Location() string { return m.Loc }
+func (m *metadata) Definitions() []*Definition { return
cloneSlice(m.DefinitionList) }
Review Comment:
This makes UDF metadata copy-on-read, which no other metadata type does —
commonMetadata returns its internals raw (Schemas() → c.SchemaList, Snapshots()
→ c.SnapshotList, Properties() → c.Props at table/metadata.go:1646/1685/1727).
The repo's contract is immutability-by-convention with the builder as the only
isolation point, and the UDF builder already honors that: AddDefinition and
AddDefinitionVersion clone their inputs (metadata_builder.go:363, :423) and
MetadataBuilderFromBase deep-copies the base.
Recommend dropping the read-path clones and matching the convention.
Copy-on-read doesn't scale to TableMetadata — those accessors sit on
read/planning paths and would start allocating per call — so keeping it here
just leaves UDF a permanent outlier for a safety property nothing else in the
repo asks for. If the actual goal is hard immutability, the Iceberg-idiomatic
route is unexported fields + constructors like Java's
ViewMetadata/TableMetadata, not per-getter copies, but that's a separate change.
--
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]
