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 caecd53. 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/deferred_snapshots_test.go:
##########
@@ -131,6 +131,210 @@ func
TestDeferredSnapshotsMaterializeForCollectionAndSerialization(t *testing.T)
assert.Equal(t, eagerRebuilt.Snapshots(), rebuilt.Snapshots())
}
+func TestMetadataBuilderFromDeferredBaseRemainsLazyThroughBuild(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+ require.NotEmpty(t, state.raw)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "builder construction must not
materialize deferred history")
Review Comment:
Fixed in caecd53. Added the locked isMaterialized helper and replaced direct
raw-state assertions throughout the file. The focused deferred-builder suite
passes under the race detector.
##########
table/deferred_snapshots_test.go:
##########
@@ -131,6 +131,210 @@ func
TestDeferredSnapshotsMaterializeForCollectionAndSerialization(t *testing.T)
assert.Equal(t, eagerRebuilt.Snapshots(), rebuilt.Snapshots())
}
+func TestMetadataBuilderFromDeferredBaseRemainsLazyThroughBuild(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+ require.NotEmpty(t, state.raw)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "builder construction must not
materialize deferred history")
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "building unchanged metadata must remain
lazy")
+ assert.Equal(t, meta.CurrentSnapshot(), rebuilt.CurrentSnapshot())
+ assert.NotEmpty(t, state.raw, "referenced snapshot access must remain
eager")
+
+ assert.Len(t, rebuilt.Snapshots(), 2)
+ assert.Nil(t, state.raw, "complete collection access must materialize
history")
+}
+
+func TestDeferredBuilderAddsSnapshotWithoutMaterializingHistory(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ current := meta.CurrentSnapshot()
+ require.NotNil(t, current)
+ added := *current
+ added.SnapshotID = 987654321
+ added.ParentSnapshotID = ¤t.SnapshotID
+ added.SequenceNumber = meta.LastSequenceNumber() + 1
+ added.TimestampMs = max(meta.LastUpdatedMillis(), current.TimestampMs)
+ 1
+ require.NoError(t, builder.AddSnapshot(&added))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, added.SnapshotID,
BranchRef))
+ assert.NotEmpty(t, state.raw, "snapshot addition and ref update must
remain lazy")
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "building metadata with an addition must
remain lazy")
+ assert.Equal(t, &added, rebuilt.SnapshotByID(added.SnapshotID))
+ assert.NotEmpty(t, state.raw)
+
+ serialized, err := json.Marshal(rebuilt)
+ require.NoError(t, err)
+ reparsed, err := ParseMetadataBytes(serialized)
+ require.NoError(t, err)
+ assert.Len(t, reparsed.Snapshots(), 3)
+ assert.Equal(t, &added, reparsed.SnapshotByID(added.SnapshotID))
+}
+
+func TestDeferredBuilderRemovesSnapshotWithoutResurrection(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ eager, err := ParseMetadataBytes(data)
+ require.NoError(t, err)
+ require.Len(t, eager.Snapshots(), 2)
+ historicalID := eager.Snapshots()[0].SnapshotID
+
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ require.NoError(t, builder.RemoveSnapshots([]int64{historicalID},
false))
+ assert.NotEmpty(t, state.raw, "snapshot removal must use the deferred
ID index")
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw, "building metadata with a removal must
remain lazy")
+ assert.Nil(t, rebuilt.SnapshotByID(historicalID))
+ assert.NotEmpty(t, state.raw, "removed lookup must not materialize
history")
+
+ serialized, err := json.Marshal(rebuilt)
+ require.NoError(t, err)
+ reparsed, err := ParseMetadataBytes(serialized)
+ require.NoError(t, err)
+ assert.Len(t, reparsed.Snapshots(), 1)
+ assert.Nil(t, reparsed.SnapshotByID(historicalID))
+}
+
+func TestDeferredBuilderCanReplaceRemovedSnapshotID(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ historical := meta.SnapshotByID(3051729675574597004)
+ require.NotNil(t, historical)
+ state := commonMetadataOf(meta).deferredSnapshots
+ require.NotNil(t, state)
+
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ require.NoError(t,
builder.RemoveSnapshots([]int64{historical.SnapshotID}, false))
+ replacement := *historical
+ replacement.ManifestList = "replacement.avro"
+ current := meta.CurrentSnapshot()
+ require.NotNil(t, current)
+ replacement.ParentSnapshotID = ¤t.SnapshotID
+ replacement.SequenceNumber = meta.LastSequenceNumber() + 1
+ replacement.TimestampMs = meta.LastUpdatedMillis() + 1
+ require.NoError(t, builder.AddSnapshot(&replacement))
+ assert.NotEmpty(t, state.raw)
+
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.Equal(t, "replacement.avro",
rebuilt.SnapshotByID(historical.SnapshotID).ManifestList)
+ serialized, err := json.Marshal(rebuilt)
+ require.NoError(t, err)
+ reparsed, err := ParseMetadataBytes(serialized)
+ require.NoError(t, err)
+ require.Len(t, reparsed.Snapshots(), 2)
+ assert.Equal(t, "replacement.avro",
reparsed.SnapshotByID(historical.SnapshotID).ManifestList)
+}
+
+func TestDeferredBuilderCloneOwnsSnapshotOverlay(t *testing.T) {
+ data := metadataWithUnreferencedSnapshot(t)
+ eager, err := ParseMetadataBytes(data)
+ require.NoError(t, err)
+ historicalID := eager.Snapshots()[0].SnapshotID
+
+ meta, err := ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ cloned := builder.clone()
+ require.NoError(t, cloned.RemoveSnapshots([]int64{historicalID}, false))
+
+ originalBuilt, err := builder.Build()
+ require.NoError(t, err)
+ cloneBuilt, err := cloned.Build()
+ require.NoError(t, err)
+ assert.NotNil(t, originalBuilt.SnapshotByID(historicalID))
+ assert.Nil(t, cloneBuilt.SnapshotByID(historicalID))
+ assert.Len(t, originalBuilt.Snapshots(), 2)
+ assert.Len(t, cloneBuilt.Snapshots(), 1)
+}
+
+func TestDeferredBuiltMetadataOwnsSnapshotOverlay(t *testing.T) {
+ meta, err :=
ParseMetadataBytesDeferredSnapshots(metadataWithUnreferencedSnapshot(t))
+ require.NoError(t, err)
+ builder, err := MetadataBuilderFromBase(meta, "")
+ require.NoError(t, err)
+ current := meta.CurrentSnapshot()
+ require.NotNil(t, current)
+ added := *current
+ added.SnapshotID = 987654321
+ added.ParentSnapshotID = ¤t.SnapshotID
+ added.SequenceNumber = meta.LastSequenceNumber() + 1
+ added.TimestampMs = max(meta.LastUpdatedMillis(), current.TimestampMs)
+ 1
+ require.NoError(t, builder.AddSnapshot(&added))
+
+ built, err := builder.Build()
+ require.NoError(t, err)
+ require.NoError(t, builder.RemoveSnapshots([]int64{added.SnapshotID},
false))
+ assert.NotNil(t, built.SnapshotByID(added.SnapshotID))
+ assert.Len(t, built.Snapshots(), 3)
+ _, err = builder.SnapshotByID(added.SnapshotID)
+ assert.ErrorIs(t, err, ErrSnapshotNotFound)
+}
+
+func TestDeferredBuilderRoundTripAllMetadataVersions(t *testing.T) {
+ for _, tc := range []struct {
+ name string
+ data string
+ }{
+ {name: "v1", data: ExampleTableMetadataV1},
+ {name: "v2", data: ExampleTableMetadataV2},
+ {name: "v3", data: ExampleTableMetadataV3},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ var fields map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(tc.data),
&fields))
+ fields["refs"] = json.RawMessage(`{}`)
+ data, err := json.Marshal(fields)
+ require.NoError(t, err)
+
+ eager, err := ParseMetadataBytes(data)
+ require.NoError(t, err)
+ eagerBuilder, err := MetadataBuilderFromBase(eager, "")
+ require.NoError(t, err)
+ deferred, err :=
ParseMetadataBytesDeferredSnapshots(data)
+ require.NoError(t, err)
+ state := commonMetadataOf(deferred).deferredSnapshots
+ require.NotNil(t, state)
+ builder, err := MetadataBuilderFromBase(deferred, "")
+ require.NoError(t, err)
+ const builtTimestamp = int64(1800000000000)
+ eagerBuilder.lastUpdatedMS = builtTimestamp
+ builder.lastUpdatedMS = builtTimestamp
+ eagerRebuilt, err := eagerBuilder.Build()
+ require.NoError(t, err)
+ rebuilt, err := builder.Build()
+ require.NoError(t, err)
+ assert.NotEmpty(t, state.raw)
+ assert.True(t, eagerRebuilt.Equals(rebuilt))
Review Comment:
Fixed in caecd53. Added mutate-build-rebuild-mutate coverage comparing
equality and serialized JSON with the eager path. Also added both
SetSnapshotRef cases: removed base IDs fail, while unreferenced base IDs
succeed without full materialization.
##########
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 caecd53. Immutable metadata overlays now cache the applied
collection with sync.Once. The test calls snapshotsForMarshal twice and
verifies the cached backing collection is reused.
##########
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 caecd53. 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.
--
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]
