technicolorbeat commented on code in PR #2186:
URL: https://github.com/apache/iceberg-go/pull/2186#discussion_r4274992858
##########
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](https://github.com/apache/iceberg-go/commit/caecd53f2a6642090eba8a101191ed2b8a2aafa2).
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_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](https://github.com/apache/iceberg-go/commit/caecd53f2a6642090eba8a101191ed2b8a2aafa2).
Added the locked isMaterialized helper and replaced direct raw-state
assertions throughout the file. The focused deferred-builder suite passes under
the race detector.
--
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]
