This is an automated email from the ASF dual-hosted git repository.
laskoviymishka pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new d9bf431d5 fix(view): stop MetadataBuilder.Build from growing the
version log (#2109)
d9bf431d5 is described below
commit d9bf431d5a634884ad4b3c0078bc808ac15533c0
Author: Animesh Kumar <[email protected]>
AuthorDate: Sun Oct 11 23:45:38 2026 +0530
fix(view): stop MetadataBuilder.Build from growing the version log (#2109)
* fix(view): stop MetadataBuilder.Build from growing the version log
view.MetadataBuilder.Build appended the pending history entry to
b.versionLog, so building the same builder again added the entry again:
three builds gave three version-log entries. Append to a copy instead.
On the table side, updateSnapshotLog wrote the pruned log back into
b.snapshotLog. Running it again gives the same result, so this did not
change any output, but it was still a side effect of Build. It now
returns the pruned log (as updatedSnapshotLog) and leaves the builder
unchanged, like the metadata log since #2095.
lastUpdatedMS stays cached on the builder after the first Build, so
repeated builds report the same timestamp; that is now documented.
Closes #2108
Signed-off-by: Animesh Kumar <[email protected]>
* fix: make repeated builds stable and pin the pruning and expiry paths
- table: compute lastUpdatedMS per Build instead of storing it on the
builder, so a build after more changes gets a fresh timestamp.
- view: keep the UUID generated for a new view on the builder and reuse
it, so repeated builds describe the same view; SetUUID still works.
- Document on both Build methods that they can be called more than once.
- Tests: build twice after RemoveSnapshots, the view expiry path with
VersionHistorySizeKey=1, a builder from existing metadata, a stable
generated UUID, and a fresh timestamp after more changes.
* test(view): use the embedded Metadata methods directly
* fix: keep Build's guarantee scoped to the logs
Per review, Build only guarantees that it doesn't write the version log
or the snapshot log back to the builder.
- table: keep lastUpdatedMS cached on the builder, as Java's
TableMetadata.Builder does, and say when it is set.
- view: keep generating the UUID per build when none is assigned, as
Java's ViewMetadata.Builder does.
- Tests: drop the timestamp and UUID assertions, build a three-entry
base version log with a history size of 2, and pin the [A, A] snapshot
log after moving main back to A between builds.
- Say which calls move lastUpdatedMS after the first Build.
- Document that updatedSnapshotLog never aliases b.snapshotLog and
derives pruning from b.updates.
- Fix the [A, A] explanation and compare it with UpdateTableMetadata.
- Rename the view expiry test, pin its one-entry log, and point to the
from-base test for the clear branch.
---------
Signed-off-by: Animesh Kumar <[email protected]>
---
table/metadata.go | 27 ++++++++---
table/metadata_builder_internal_test.go | 52 +++++++++++++++++++++
view/metadata_builder.go | 9 ++--
view/metadata_builder_test.go | 82 +++++++++++++++++++++++++++++++++
4 files changed, 160 insertions(+), 10 deletions(-)
diff --git a/table/metadata.go b/table/metadata.go
index bf1dc9e26..4b26e898f 100644
--- a/table/metadata.go
+++ b/table/metadata.go
@@ -1064,7 +1064,7 @@ func (b *MetadataBuilder) RemoveSnapshots(snapshotIds
[]int64, postCommit bool)
if len(b.snapshotList) != previousSnapshotCount {
b.snapshotIndex = buildSnapshotIndex(b.snapshotList)
}
- // Snapshot-log pruning is deferred to updateSnapshotLog during Build so
+ // Snapshot-log pruning is deferred to updatedSnapshotLog during Build
so
// removed entries remain available when trimming history gaps.
validSnapshotIDs := make(map[int64]struct{}, len(b.snapshotList))
@@ -1509,10 +1509,16 @@ 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. After that only
AddSnapshot,
+ // SetSnapshotRef on main and SetLastUpdatedMS move it: a later
SetProperties or
+ // AddSchema followed by another Build still reports the first Build's
time.
if b.lastUpdatedMS == 0 {
b.lastUpdatedMS = time.Now().UnixMilli()
}
@@ -1544,7 +1550,7 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
SnapshotList: b.snapshotList,
snapshotIndex: b.snapshotIndex,
CurrentSnapshotID: b.currentSnapshotID,
- SnapshotLog: b.snapshotLog,
+ SnapshotLog: snapshotLog,
MetadataLog: metadataLog,
SortOrderList: b.sortOrderList,
DefaultSortOrderID: b.defaultSortOrderID,
@@ -1555,7 +1561,13 @@ func (b *MetadataBuilder) buildCommonMetadata()
(*commonMetadata, error) {
}, nil
}
-func (b *MetadataBuilder) updateSnapshotLog() error {
+// updatedSnapshotLog returns the snapshot log with intermediate and removed
+// snapshots dropped. It leaves b.snapshotLog unchanged, so Build stays free of
+// side effects on the log. The result is always a new slice that never aliases
+// b.snapshotLog, including the slices.Clone when nothing is pruned, so callers
+// may modify it. The pruning is derived again from b.updates on every call, so
+// it relies on b.updates holding every change made through this builder.
+func (b *MetadataBuilder) updatedSnapshotLog() ([]SnapshotLogEntry, error) {
addedIDs := make(map[int64]struct{}, 2)
hasRemoved := false
for _, upd := range b.updates {
@@ -1593,13 +1605,14 @@ func (b *MetadataBuilder) updateSnapshotLog() error {
if b.currentSnapshotID != nil && len(newSnapsLog) != 0 {
last := newSnapsLog[len(newSnapsLog)-1]
if last.SnapshotID != *b.currentSnapshotID {
- return fmt.Errorf("%w: cannot set invalid
snapshot log: latest entry is not the current snapshot",
iceberg.ErrInvalidArgument)
+ return nil, fmt.Errorf("%w: cannot set invalid
snapshot log: latest entry is not the current snapshot",
iceberg.ErrInvalidArgument)
}
}
- b.snapshotLog = newSnapsLog
+
+ return newSnapsLog, nil
}
- return nil
+ return slices.Clone(b.snapshotLog), nil
}
func (b *MetadataBuilder) GetSchemaByID(id int) (*iceberg.Schema, error) {
diff --git a/table/metadata_builder_internal_test.go
b/table/metadata_builder_internal_test.go
index 78b79298e..5030647e2 100644
--- a/table/metadata_builder_internal_test.go
+++ b/table/metadata_builder_internal_test.go
@@ -1060,6 +1060,52 @@ 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.
+ // In the first build main moved from A to B, so A is the intermediate
snapshot and
+ // that build logs [B]. Moving main back to A makes B the intermediate
one, while
+ // both of A's entries stay, so the second build logs [A, A]. Before
the fix, the
+ // first build wrote its pruned [B] back to the builder and the second
returned [A].
+ builder := builderWithoutChanges(2)
+ schemaID := 0
+ newSnapshot := func(id int64) *Snapshot {
+ return &Snapshot{
+ SnapshotID: id,
+ TimestampMs: builder.base.LastUpdatedMillis() + id,
+ ManifestList: fmt.Sprintf("/snap-%d.avro", id),
+ Summary: &Summary{Operation: OpAppend},
+ SchemaID: &schemaID,
+ }
+ }
+
+ require.NoError(t, builder.AddSnapshot(newSnapshot(1)))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef))
+ require.NoError(t, builder.AddSnapshot(newSnapshot(2)))
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 2, BranchRef))
+ _, err := builder.Build()
+ require.NoError(t, err)
+
+ require.NoError(t, builder.SetSnapshotRef(MainBranch, 1, BranchRef))
+ res, err := builder.Build()
+ require.NoError(t, err)
+
+ ids := make([]int64, 0, 2)
+ for entry := range res.SnapshotLogs() {
+ ids = append(ids, entry.SnapshotID)
+ }
+ require.Equal(t, []int64{1, 1}, ids)
+
+ // Committing the same updates gives the same log.
+ committed, err := UpdateTableMetadata(builder.base, builder.updates, "")
+ require.NoError(t, err)
+ require.Equal(t, slices.Collect(committed.SnapshotLogs()),
slices.Collect(res.SnapshotLogs()))
}
func TestRemoveSnapshotsPrunesSnapshotLogHistory(t *testing.T) {
@@ -1104,6 +1150,12 @@ func TestRemoveSnapshotsPrunesSnapshotLogHistory(t
*testing.T) {
SnapshotID: snapshot3ID,
TimestampMs: baseTimestamp + 3,
}}, slices.Collect(rebuilt.SnapshotLogs()))
+
+ // Building again prunes the same entries from the builder's log, which
keeps them.
+ again, err := newBuilder.Build()
+ require.NoError(t, err)
+ require.Equal(t, slices.Collect(rebuilt.SnapshotLogs()),
slices.Collect(again.SnapshotLogs()))
+ require.Len(t, newBuilder.snapshotLog, 3, "Build must not prune the
builder's own log")
}
func TestSetBranchSnapshotCreatesBranchIfNotExists(t *testing.T) {
diff --git a/view/metadata_builder.go b/view/metadata_builder.go
index d00018e56..3f2190748 100644
--- a/view/metadata_builder.go
+++ b/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
+ // appended to a copy. Appending to b.versionLog would add it again on
every call.
+ versionLog := slices.Clone(b.versionLog)
if b.versionHistoryEntry != nil {
- b.versionLog = append(b.versionLog, *b.versionHistoryEntry)
+ versionLog = append(versionLog, *b.versionHistoryEntry)
}
// If we had a previous version, check if we allow dropping dialects
and if we did drop one
@@ -459,10 +462,10 @@ func (b *MetadataBuilder) Build() (*MetadataBuildResult,
error) {
if len(b.versionList) > versionsToKeep {
retainedVersions = expireVersions(b.versionsById,
versionsToKeep, b.versionsById[b.currentVersionID])
retainedVersionIDs :=
internal.ToSet(internal.MapSlice(retainedVersions, func(v *Version) int64 {
return v.VersionID }))
- retainedHistory = updateHistory(b.versionLog,
retainedVersionIDs)
+ retainedHistory = updateHistory(versionLog, retainedVersionIDs)
} else {
retainedVersions = b.versionList
- retainedHistory = b.versionLog
+ retainedHistory = versionLog
}
md, err := b.buildMetadata(retainedVersions, retainedHistory)
diff --git a/view/metadata_builder_test.go b/view/metadata_builder_test.go
index 335ecbbb5..753bed0df 100644
--- a/view/metadata_builder_test.go
+++ b/view/metadata_builder_test.go
@@ -97,6 +97,88 @@ 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_RepeatedBuildsWithVersionExpiry(t *testing.T) {
+ // With a history size of 1, versions 1 and 2 are expired. The only
history entry is
+ // the one for version 3, which is retained, so updateHistory keeps it;
its clear
+ // branch is covered by TestBuild_FromBaseDoesNotGrowVersionLog.
Repeated builds
+ // return the same one-entry log.
+ b := newTestBuilder().
+ SetLoc("location").
+ SetProperties(iceberg.Properties{VersionHistorySizeKey: "1"}).
+ AddSchema(newTestSchema(1)).
+ AddVersion(newTestVersionWithSQL(1, LastAddedID, "select 1")).
+ AddVersion(newTestVersionWithSQL(2, LastAddedID, "select 2")).
+ AddVersion(newTestVersionWithSQL(3, LastAddedID, "select 3")).
+ SetCurrentVersionID(LastAddedID)
+
+ first, err := b.Build()
+ require.NoError(t, err)
+ require.Len(t, first.VersionLog(), 1)
+ require.Equal(t, int64(3), first.VersionLog()[0].VersionID)
+ for range 2 {
+ res, err := b.Build()
+ require.NoError(t, err)
+ require.Equal(t, first.VersionLog(), res.VersionLog())
+ require.Empty(t, b.versionLog, "Build must not append to the
builder's own log")
+ }
+}
+
+func TestBuild_FromBaseDoesNotGrowVersionLog(t *testing.T) {
+ // The commit-retry case: a builder from existing metadata with a
three-entry version
+ // log and a history size of 2, replacing the current version, built
more than once.
+ res, err := newTestBuilder().
+ SetLoc("location").
+ AddSchema(newTestSchema(1)).
+ AddVersion(newTestVersionWithSQL(1, LastAddedID, "select 1")).
+ SetCurrentVersionID(LastAddedID).
+ Build()
+ require.NoError(t, err)
+ for id := int64(2); id <= 3; id++ {
+ b, err := MetadataBuilderFromBase(res.Metadata)
+ require.NoError(t, err)
+ schemaID := res.CurrentVersion().SchemaID
+ res, err = b.AddVersion(newTestVersionWithSQL(id, schemaID,
fmt.Sprintf("select %d", id))).
+ SetCurrentVersionID(LastAddedID).
+ Build()
+ require.NoError(t, err)
+ }
+ baseLog := res.VersionLog()
+ require.Len(t, baseLog, 3)
+
+ b, err := MetadataBuilderFromBase(res.Metadata)
+ require.NoError(t, err)
+ schemaID := res.CurrentVersion().SchemaID
+ b.SetProperties(iceberg.Properties{VersionHistorySizeKey: "2"}).
+ AddVersion(newTestVersionWithSQL(4, schemaID, "select 4")).
+ SetCurrentVersionID(LastAddedID)
+
+ for range 3 {
+ res, err := b.Build()
+ require.NoError(t, err)
+ ids := make([]int64, 0, 2)
+ for _, entry := range res.VersionLog() {
+ ids = append(ids, entry.VersionID)
+ }
+ require.Equal(t, []int64{3, 4}, ids)
+ require.Equal(t, baseLog, b.versionLog, "Build must not append
to the builder's own log")
+ }
+}
+
func TestNewVersion_RepresentationValidation(t *testing.T) {
tests := []struct {
name string
