CaptainAni187 commented on code in PR #2109:
URL: https://github.com/apache/iceberg-go/pull/2109#discussion_r4210450473
##########
table/metadata.go:
##########
@@ -1509,10 +1509,15 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
}
defaultSpecID := b.defaultSpecID
- if err := b.updateSnapshotLog(); err != nil {
+ snapshotLog, err := b.updatedSnapshotLog()
+ if err != nil {
return nil, fmt.Errorf("%w: %w", ErrInvalidMetadata, err)
}
+ // If no change has set lastUpdatedMS yet, the first Build sets it and
keeps it on
+ // the builder on purpose (Java's TableMetadata.Builder does the same),
so repeated
+ // builds of the same changes report the same time. Adding a snapshot
or moving
Review Comment:
Narrowed in 14e433a. The comment now says that after the first Build only
`AddSnapshot`, `SetSnapshotRef` on main and `SetLastUpdatedMS` move the
timestamp. A later `SetProperties` or `AddSchema` followed by another Build
still reports the first Build's time.
##########
table/metadata.go:
##########
@@ -1555,7 +1560,10 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
}, nil
}
-func (b *MetadataBuilder) updateSnapshotLog() error {
+// updatedSnapshotLog returns the snapshot log with intermediate and removed
Review Comment:
Added to the doc in 14e433a: the result is always a new slice that never
aliases `b.snapshotLog`, including the `slices.Clone` fall-through, and pruning
is derived again from `b.updates` on every call, so it relies on `b.updates`
holding every change made through the builder.
##########
table/metadata_builder_internal_test.go:
##########
@@ -1060,6 +1060,45 @@ func TestSnapshotLogSkipsIntermediate(t *testing.T) {
TimestampMs: snapshot2.TimestampMs,
}, "expected snapshot to match added snapshot")
require.True(t, res.CurrentSnapshot().Equals(snapshot2))
+
+ require.Len(t, builder.snapshotLog, 2, "Build must not prune the
builder's own log")
+ again, err := builder.Build()
+ require.NoError(t, err)
+ require.Equal(t, res.(*metadataV2).SnapshotLog,
again.(*metadataV2).SnapshotLog)
+}
+
+func TestSnapshotLogAfterMovingMainBackAfterBuild(t *testing.T) {
+ // add A, set main to A, add B, set main to B, build, set main back to
A, build.
+ // B is intermediate in the first build, so it is skipped, and the
second build
Review Comment:
You're right, the comment had it backwards. In 14e433a it now says A is the
intermediate snapshot in the first build, which logs [B]. Moving main back to A
makes B intermediate while both of A's entries stay, hence [A, A]. The test
also commits `builder.updates` through `UpdateTableMetadata` and checks the
logs are equal.
##########
view/metadata_builder_test.go:
##########
@@ -97,6 +97,84 @@ func TestBuild_NullAndMissingFields(t *testing.T) {
assert.ErrorContains(t, err, "cannot set uuid to null")
}
+func TestBuild_DoesNotGrowVersionLog(t *testing.T) {
+ b := newTestBuilder().
+ SetLoc("location").
+ AddSchema(newTestSchema(1)).
+ AddVersion(newTestVersion(1, LastAddedID)).
+ SetCurrentVersionID(LastAddedID)
+
+ for range 3 {
+ res, err := b.Build()
+ require.NoError(t, err)
+ require.Len(t, res.VersionLog(), 1)
+ require.Empty(t, b.versionLog, "Build must not append to the
builder's own log")
+ }
+}
+
+func TestBuild_DoesNotGrowVersionLogWhenExpiringVersions(t *testing.T) {
Review Comment:
Renamed to `TestBuild_RepeatedBuildsWithVersionExpiry` in 14e433a. The
comment now says it keeps the retained entry and points to
`TestBuild_FromBaseDoesNotGrowVersionLog` for the clear branch. The test also
asserts the first log is exactly one entry for version 3.
--
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]
