You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ze...@apache.org on 2026/03/16 22:16:43 UTC
(iceberg-go) branch main updated: fix(table): removeSnaphosts postCommit now takes effect (#783)
This is an automated email from the ASF dual-hosted git repository.
zeroshade 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 de373a56 fix(table): removeSnaphosts postCommit now takes effect (#783)
de373a56 is described below
commit de373a56ec8dae21860d868193799ac43cadf828
Author: starpact <58...@users.noreply.github.com>
AuthorDate: Tue Mar 17 06:16:36 2026 +0800
fix(table): removeSnaphosts postCommit now takes effect (#783)
`postCommit` of `removeSnapshotsUpdate` is ignored when applied to
`MetadataBuilder`, so it actually never takes effect.
https://github.com/apache/iceberg-go/blob/a012a1770d70e0df6babcb63e42058f484390602/table/metadata.go#L508
I made postCommit part of the constructor, as otherwise we have to:
https://github.com/apache/iceberg-go/blob/a012a1770d70e0df6babcb63e42058f484390602/table/transaction.go#L300-L301
which seems pretty error-prone. Though this is technically a breaking
change for a somewhat internal API.
---
table/metadata.go | 4 ++--
table/metadata_builder_internal_test.go | 2 +-
table/transaction.go | 4 +---
table/updates.go | 5 +++--
table/updates_test.go | 5 ++---
5 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/table/metadata.go b/table/metadata.go
index 5d303c45..09db1481 100644
--- a/table/metadata.go
+++ b/table/metadata.go
@@ -485,7 +485,7 @@ func (b *MetadataBuilder) NextRowID() int64 {
return b.currentNextRowID()
}
-func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64) error {
+func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64, postCommit bool) error {
if b.currentSnapshotID != nil && slices.Contains(snapshotIds, *b.currentSnapshotID) {
return errors.New("current snapshot cannot be removed")
}
@@ -505,7 +505,7 @@ func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64) error {
}
b.refs = newRefs
- b.updates = append(b.updates, NewRemoveSnapshotsUpdate(snapshotIds))
+ b.updates = append(b.updates, NewRemoveSnapshotsUpdate(snapshotIds, postCommit))
return nil
}
diff --git a/table/metadata_builder_internal_test.go b/table/metadata_builder_internal_test.go
index 6c9fed32..79cca000 100644
--- a/table/metadata_builder_internal_test.go
+++ b/table/metadata_builder_internal_test.go
@@ -609,7 +609,7 @@ func TestRemoveSnapshotRemovesBranch(t *testing.T) {
newBuilder, err := MetadataBuilderFromBase(meta, "")
require.NoError(t, err)
- require.NoError(t, newBuilder.RemoveSnapshots([]int64{snapshot.SnapshotID}))
+ require.NoError(t, newBuilder.RemoveSnapshots([]int64{snapshot.SnapshotID}, false))
newMeta, err := newBuilder.Build()
require.NoError(t, err)
require.NotNil(t, newMeta)
diff --git a/table/transaction.go b/table/transaction.go
index c946c17b..860b70e2 100644
--- a/table/transaction.go
+++ b/table/transaction.go
@@ -297,9 +297,7 @@ func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
// Only add the update if there are actually snapshots to delete
if len(snapsToDelete) > 0 {
- update := NewRemoveSnapshotsUpdate(snapsToDelete)
- update.postCommit = cfg.postCommit
- updates = append(updates, update)
+ updates = append(updates, NewRemoveSnapshotsUpdate(snapsToDelete, cfg.postCommit))
}
return t.apply(updates, reqs)
diff --git a/table/updates.go b/table/updates.go
index ad302a29..f79fe00e 100644
--- a/table/updates.go
+++ b/table/updates.go
@@ -417,15 +417,16 @@ type removeSnapshotsUpdate struct {
// NewRemoveSnapshotsUpdate creates a new update that removes all snapshots from
// the table metadata with the given snapshot IDs.
-func NewRemoveSnapshotsUpdate(ids []int64) *removeSnapshotsUpdate {
+func NewRemoveSnapshotsUpdate(ids []int64, postCommit bool) *removeSnapshotsUpdate {
return &removeSnapshotsUpdate{
baseUpdate: baseUpdate{ActionName: UpdateRemoveSnapshots},
SnapshotIDs: ids,
+ postCommit: postCommit,
}
}
func (u *removeSnapshotsUpdate) Apply(builder *MetadataBuilder) error {
- return builder.RemoveSnapshots(u.SnapshotIDs)
+ return builder.RemoveSnapshots(u.SnapshotIDs, u.postCommit)
}
func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table, postTable *Table) error {
diff --git a/table/updates_test.go b/table/updates_test.go
index 831abb4b..c7d5c961 100644
--- a/table/updates_test.go
+++ b/table/updates_test.go
@@ -29,8 +29,7 @@ import (
)
func TestRemoveSnapshotsPostCommitSkipped(t *testing.T) {
- update := NewRemoveSnapshotsUpdate([]int64{1, 2, 3})
- update.postCommit = false
+ update := NewRemoveSnapshotsUpdate([]int64{1, 2, 3}, false)
// PostCommit should return nil immediately when postCommit is false,
// without accessing the table arguments (which are nil here)
@@ -90,7 +89,7 @@ func TestUnmarshalUpdates(t *testing.T) {
NewRemovePropertiesUpdate([]string{"key2"}),
NewRemoveSchemasUpdate([]int{1, 2, 3, 4}),
NewRemoveSpecUpdate([]int{1, 2, 3}),
- NewRemoveSnapshotsUpdate([]int64{1, 2}),
+ NewRemoveSnapshotsUpdate([]int64{1, 2}, false),
NewRemoveSnapshotRefUpdate("main"),
NewSetDefaultSortOrderUpdate(1),
NewSetDefaultSpecUpdate(1),