zeroshade commented on code in PR #2109:
URL: https://github.com/apache/iceberg-go/pull/2109#discussion_r4198577368
##########
table/metadata_builder_internal_test.go:
##########
@@ -1060,6 +1060,11 @@ 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")
Review Comment:
Worth adding, though the premise is off. `TestSnapshotLogSkipsIntermediate`
already takes the pruning branch (snapshot 1 is intermediate), and its new
assertion fails on main. Adding the same build-twice check to
`TestRemoveSnapshotsPrunesSnapshotLogHistory` pins the `hasRemoved` arm. I
tried it: it passes here and fails on main.
<sub>Drafted with an AI-assisted tool; confirmed by a maintainer.</sub>
##########
view/metadata_builder.go:
##########
@@ -435,8 +435,11 @@ func (b *MetadataBuilder) Build() (*MetadataBuildResult,
error) {
return nil, fmt.Errorf("unsupported format version %d",
b.formatVersion)
}
+ // Build may run more than once on the same builder, so the history
entry is
Review Comment:
Out of scope for this PR. Java's `ViewMetadata.Builder.build()` also
generates a fresh UUID on every build when none is assigned. Caching it would
make `Build()` write builder state again. No `AssignUUID` change would be
recorded either, so replaying `Changes` would still produce a different UUID.
The production callers (`createView`, `NewMetadataWithUUID`) build once. A
separate issue is fine if we want stable UUIDs.
<sub>Drafted with an AI-assisted tool; confirmed by a maintainer.</sub>
##########
view/metadata_builder_test.go:
##########
@@ -97,6 +97,21 @@ 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)
Review Comment:
Agreed, minus the UUID assertion. Add a `MetadataBuilderFromBase` case with
a non-empty `versionLog`. Setting `VersionHistorySizeKey` low in the same test
also covers the expiry branch. I tried a three-entry base log with history size
2: each of three builds returns `[3, 4]` and leaves the builder's log at three
entries, while main grows it on the first build. Without `SetUUID` the UUIDs
differ by design, so don't assert they match.
<sub>Drafted with an AI-assisted tool; confirmed by a maintainer.</sub>
--
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]
