laskoviymishka commented on code in PR #2109:
URL: https://github.com/apache/iceberg-go/pull/2109#discussion_r4195549575
##########
table/metadata.go:
##########
@@ -1509,10 +1509,13 @@ 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)
}
+ // The timestamp is fixed by the first Build and kept on the builder on
purpose, so
+ // repeated builds of the same changes report the same last-updated
time.
Review Comment:
I'd either compute `lastUpdatedMS` into a local here instead of caching it
on the builder, or keep the cache and have the comment say the builder is
single-shot once built. As written it's the one side effect Build still keeps,
and it contradicts the `updatedSnapshotLog` doc that now says Build is
side-effect-free: build, add a snapshot, build again reuses the first
timestamp, which can end up earlier than the snapshot it just added.
##########
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:
Nice that this double-builds and pins `builder.snapshotLog` at 2. The gap is
it only covers the no-op path — the branch that actually changed is the pruning
one in `updatedSnapshotLog`, and nothing builds twice after `RemoveSnapshots`.
I'd add the same build-twice-and-compare to
`TestRemoveSnapshotsPrunesSnapshotLogHistory` and check `builder.snapshotLog`
keeps its removed entries.
##########
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:
Not introduced here, but it undercuts the same goal: `buildMetadata` just
above generates `uuid.New()` into a local when `b.uuid` is nil and never stores
it, so repeated builds of the same view hand back different UUIDs — a retried
commit builds a different view each time. Since this PR is making repeated
builds stable, I'd cache the generated UUID onto `b.uuid` here too, or document
that it's deliberately per-build.
##########
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:
This only exercises the no-expiry path — one version, so `len(b.versionList)
> versionsToKeep` stays false and `updateHistory` (where the clone fix also
applies) never runs. I'd add a case with `VersionHistorySizeKey=1` and several
versions, plus one built from existing metadata with a non-empty initial
`versionLog`, since that's the real commit-retry scenario. Worth asserting the
UUID is equal across the three builds here too.
--
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]
