laskoviymishka commented on code in PR #2109:
URL: https://github.com/apache/iceberg-go/pull/2109#discussion_r4207021451
##########
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:
The property this whole fix leans on is that the result is always a fresh
slice that never aliases `b.snapshotLog`. I'd state that in the doc so nobody
optimizes the fall-through `slices.Clone` away later. Worth noting too that
pruning is re-derived from `b.updates` on every Build, so the output quietly
depends on `b.updates` staying the full change history (a future
reset-after-commit would silently change it).
##########
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:
The last sentence claims more than the code does. Only `AddSnapshot` and
`SetSnapshotRef` set `lastUpdatedMS`, so after a first Build a non-snapshot
edit (`AddSchema`, `SetProperties`) followed by a second Build still reports
the first build's time. I'd narrow it to say the timestamp is latched by the
first Build unless a snapshot or ref change moves it, so nobody reads it as
"any later mutation refreshes it."
##########
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:
The comment says B is intermediate "in the first build," but there it's A
that was current and then replaced by B, so A is the one collapsed in that
build; B only becomes the skipped intermediate once main moves back to A for
the second build. The `[1, 1]` assertion looks right; it's the explanation that
names the wrong snapshot, and as written the next reader can't tell `[A, A]` is
deliberate. While rewording, running the same sequence through
`UpdateTableMetadata` and comparing logs would actually pin the parity the
comment claims.
##########
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:
The comment says `updateHistory` prunes the log on every Build, but with a
fresh builder the log only ever holds the entry for version 3, the retained
version, so it always takes the retained branch and the clear path never runs.
It passes for the right reason (no growth), but it doesn't prove pruning on the
clone; `TestBuild_FromBaseDoesNotGrowVersionLog` is the one that actually
exercises the clear branch. I'd either fold this into the from-base case or
rename it and fix the comment, and assert the exact first log (`Len` 1,
`VersionID` 3) so it pins what it's really checking.
--
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]
