technicolorbeat commented on code in PR #2186:
URL: https://github.com/apache/iceberg-go/pull/2186#discussion_r4274992563
##########
table/metadata.go:
##########
@@ -515,11 +517,14 @@ func MetadataBuilderFromBase(metadata Metadata,
currentFileLocation string) (*Me
if b.props == nil {
b.props = iceberg.Properties{}
}
- snapshots, err := common.snapshotsForMarshal()
- if err != nil {
- return nil, err
+ b.snapshotList = cloneSnapshots(common.SnapshotList)
Review Comment:
Fixed in
[](https://github.com/apache/iceberg-go/commit/caecd53f2a6642090eba8a101191ed2b8a2aafa2).
ExpireSnapshots now calls snapshotsForFullScan, which intentionally
materializes the complete logical history before both candidate loops.
TestExpireSnapshotsMaterializesDeferredHistory parses deferred metadata and
verifies the historical ID is included in removeSnapshotsUpdate.
##########
table/metadata.go:
##########
@@ -973,6 +990,9 @@ func (b *MetadataBuilder) addSnapshotInternal(snapshot
*Snapshot, preserveUpdate
b.snapshotList = append(b.snapshotList, *snapshot)
b.snapshotIndex.positions[snapshot.SnapshotID] = len(b.snapshotList) - 1
b.snapshotIndex.firstSnapshot = snapshotListFirst(b.snapshotList)
+ if b.snapshotOverlay != nil {
+ b.snapshotOverlay.add(*snapshot)
Review Comment:
Fixed in
[](https://github.com/apache/iceberg-go/commit/caecd53f2a6642090eba8a101191ed2b8a2aafa2).
The builder no longer maintains overlay additions. snapshotList is the single
mutable source of truth, and Build derives additions as entries absent from the
deferred base or entries whose base ID is tombstoned for remove-then-readd.
##########
table/deferred_snapshots.go:
##########
@@ -41,6 +43,69 @@ type deferredSnapshotState struct {
err error
}
+// deferredSnapshotOverlay records mutations over an immutable deferred
+// snapshot state. Builders own a mutable overlay and clone it when producing
+// metadata so built metadata cannot observe subsequent builder changes.
+type deferredSnapshotOverlay struct {
+ additions []Snapshot
+ removed map[int64]struct{}
+}
+
+func (o *deferredSnapshotOverlay) clone() *deferredSnapshotOverlay {
+ if o == nil {
+ return nil
+ }
+
+ return &deferredSnapshotOverlay{
+ additions: cloneSnapshots(o.additions),
+ removed: maps.Clone(o.removed),
+ }
+}
+
+func (o *deferredSnapshotOverlay) isRemoved(id int64) bool {
+ if o == nil {
+ return false
+ }
+
+ _, ok := o.removed[id]
+
+ return ok
+}
+
+func (o *deferredSnapshotOverlay) add(snapshot Snapshot) {
+ o.additions = append(o.additions, snapshot)
+}
+
+func (o *deferredSnapshotOverlay) remove(ids map[int64]struct{}) {
+ if o.removed == nil {
+ o.removed = make(map[int64]struct{}, len(ids))
+ }
+ for id := range ids {
+ o.removed[id] = struct{}{}
+ }
+ o.additions = slices.DeleteFunc(o.additions, func(snapshot Snapshot)
bool {
+ _, ok := ids[snapshot.SnapshotID]
+
+ return ok
+ })
+}
+
+func (o *deferredSnapshotOverlay) apply(base []Snapshot) []Snapshot {
+ if o == nil || (len(o.additions) == 0 && len(o.removed) == 0) {
+ return base
+ }
+
+ snapshots := make([]Snapshot, 0, len(base)+len(o.additions))
Review Comment:
Fixed in
[](https://github.com/apache/iceberg-go/commit/caecd53f2a6642090eba8a101191ed2b8a2aafa2).
Immutable metadata overlays now cache the applied collection with sync.Once.
The test calls snapshotsForMarshal twice and verifies the cached backing
collection is reused.
--
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]
