tanmayrauth commented on code in PR #1416:
URL: https://github.com/apache/iceberg-go/pull/1416#discussion_r3525973264
##########
table/metadata.go:
##########
@@ -283,6 +283,59 @@ func MetadataBuilderFromBase(metadata Metadata,
currentFileLocation string) (*Me
return b, nil
}
+func cloneMetadataBuilderInt[T any](value *T) *T {
+ if value == nil {
+ return nil
+ }
+
+ cloned := *value
+
+ return &cloned
+}
+
+// clone returns a metadata-builder copy suitable for staging updates.
+// The copy shares only immutable data with the original builder and keeps
+// value types separated so partial update attempts remain atomic.
+func (b *MetadataBuilder) clone() *MetadataBuilder {
+ if b == nil {
+ return nil
+ }
+
+ cloned := &MetadataBuilder{
Review Comment:
Suggestion: this manually lists all 29 fields, so a newly-added scalar field
would silently clone as its zero value with no compiler warning — an easy
future bug. Consider `cloned := *b` to auto-carry every value/scalar field,
then override just the reference types (slices/maps/pointers) below. That way
the only thing to maintain is the deep-copy list, not the full field set.
##########
table/metadata.go:
##########
@@ -283,6 +283,59 @@ func MetadataBuilderFromBase(metadata Metadata,
currentFileLocation string) (*Me
return b, nil
}
+func cloneMetadataBuilderInt[T any](value *T) *T {
+ if value == nil {
+ return nil
+ }
+
+ cloned := *value
+
+ return &cloned
+}
+
+// clone returns a metadata-builder copy suitable for staging updates.
+// The copy shares only immutable data with the original builder and keeps
+// value types separated so partial update attempts remain atomic.
+func (b *MetadataBuilder) clone() *MetadataBuilder {
+ if b == nil {
+ return nil
+ }
+
+ cloned := &MetadataBuilder{
+ base: b.base,
+ updates: append([]Update(nil), b.updates...),
+ formatVersion: b.formatVersion,
+ uuid: b.uuid,
+ loc: b.loc,
+ lastUpdatedMS: b.lastUpdatedMS,
+ lastColumnId: b.lastColumnId,
+ schemaList: slices.Clone(b.schemaList),
+ currentSchemaID: b.currentSchemaID,
+ specs: slices.Clone(b.specs),
+ defaultSpecID: b.defaultSpecID,
+ lastPartitionID: b.lastPartitionID,
Review Comment:
nit: this shallow-copies the pointer while every sibling *int/*int64
(currentSnapshotID, lastSequenceNumber, nextRowID, lastAdded*) goes through
cloneMetadataBuilderInt. It's safe today only because addPartitionSpec
reassigns `b.lastPartitionID = &x` rather than writing `*b.lastPartitionID`,
so a failed apply can't leak back into t.meta — but it's inconsistent and turns
into a real atomicity hole if anything ever mutates the pointee in place.
Suggest `cloneMetadataBuilderInt(b.lastPartitionID)` for consistency.
--
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]
