Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions table/table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,92 @@ func (t *TableWritingTestSuite) TestExpireSnapshots() {
t.Require().Equal(2, len(slices.Collect(tbl.Metadata().SnapshotLogs())))
}

func (t *TableWritingTestSuite) TestExpireSnapshotsWithMissingParent() {
// This test validates the fix for handling missing parent snapshots.
// After expiring snapshots, remaining snapshots may have parent-snapshot-id
// references pointing to snapshots that no longer exist. ExpireSnapshots should
// treat missing parents as the end of the chain rather than returning an error.
fs := iceio.LocalFS{}

files := make([]string, 0)
for i := range 5 {
filePath := fmt.Sprintf("%s/expire_with_missing_parent_v%d/data-%d.parquet", t.location, t.formatVersion, i)
t.writeParquet(fs, filePath, t.arrTablePromotedTypes)
files = append(files, filePath)
}

ident := table.Identifier{"default", "expire_with_missing_parent_v" + strconv.Itoa(t.formatVersion)}
meta, err := table.NewMetadata(t.tableSchemaPromotedTypes, iceberg.UnpartitionedSpec,
table.UnsortedSortOrder, t.location, iceberg.Properties{table.PropertyFormatVersion: strconv.Itoa(t.formatVersion)})
t.Require().NoError(err)

ctx := context.Background()

tbl := table.New(
ident,
meta,
t.getMetadataLoc(),
func(ctx context.Context) (iceio.IO, error) {
return fs, nil
},
&mockedCatalog{meta},
)

// Create 5 snapshots, each one with a parent pointing to the previous
for i := range 5 {
tx := tbl.NewTransaction()
t.Require().NoError(tx.AddFiles(ctx, files[i:i+1], nil, false))
tbl, err = tx.Commit(ctx)
t.Require().NoError(err)
}

t.Require().Equal(5, len(tbl.Metadata().Snapshots()))

// Get the snapshot IDs before expiration
snapshotsBeforeExpire := tbl.Metadata().Snapshots()
snapshot3ID := snapshotsBeforeExpire[2].SnapshotID
snapshot4ID := snapshotsBeforeExpire[3].SnapshotID

// Expire the first 3 snapshots, keeping only the last 2
tx := tbl.NewTransaction()
t.Require().NoError(tx.ExpireSnapshots(table.WithOlderThan(0), table.WithRetainLast(2)))
tbl, err = tx.Commit(ctx)
t.Require().NoError(err)
t.Require().Equal(2, len(tbl.Metadata().Snapshots()))

// Verify that the 4th snapshot's parent (snapshot 3) is no longer in the metadata
remainingSnapshots := tbl.Metadata().Snapshots()
var snapshot4 *table.Snapshot
for i := range remainingSnapshots {
if remainingSnapshots[i].SnapshotID == snapshot4ID {
snapshot4 = &remainingSnapshots[i]

break
}
}
t.Require().NotNil(snapshot4, "snapshot 4 should still exist")
t.Require().NotNil(snapshot4.ParentSnapshotID, "snapshot 4 should have a parent ID")
t.Require().Equal(snapshot3ID, *snapshot4.ParentSnapshotID, "snapshot 4's parent should be snapshot 3")

// Verify snapshot 3 is no longer in the metadata
t.Nil(tbl.Metadata().SnapshotByID(snapshot3ID), "snapshot 3 should have been removed")

// At this point, the 4th snapshot has a parent-snapshot-id pointing to
// the 3rd snapshot which no longer exists. Try to expire again - this
// should not fail even though the parent is missing. Use WithRetainLast(2)
// to force walking the full parent chain.
tx = tbl.NewTransaction()
// This should succeed without error despite the missing parent.
// WithRetainLast(2) will cause it to walk back through snapshot 4's parent chain,
// encountering the missing snapshot 3.
err = tx.ExpireSnapshots(table.WithOlderThan(0), table.WithRetainLast(2))
t.Require().NoError(err, "ExpireSnapshots should handle missing parent gracefully")

tbl, err = tx.Commit(ctx)
t.Require().NoError(err)
t.Require().Equal(2, len(tbl.Metadata().Snapshots()), "should still have 2 snapshots")
}

func (t *TableWritingTestSuite) TestWriteSpecialCharacterColumn() {
ident := table.Identifier{"default", "write_special_character_column"}
colNameWithSpecialChar := "letter/abc"
Expand Down
4 changes: 3 additions & 1 deletion table/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,9 @@ func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
for {
snap, err := t.meta.SnapshotByID(snapId)
if err != nil {
return err
// Parent snapshot may have been removed by a previous expiration.
// Treat missing parent as end of chain - this is expected behavior.
break
Comment on lines 243 to +246
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we match on the specific error type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@twuebi not sure I understand. There's really only one reason SnapshotByID() fails -- when the given snapshot ID doesn't exist in snapshotList list.

Copy link
Contributor

@twuebi twuebi Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably goes beyond the scope of this PR, if the only reason for SnapshotByID to ever error is not found, then it should probably become a bool, ptr or simply just ptr interface.

Relying on the function to only return an err for not found means that we rely on something that's not statically checked, although unlikely, someone may change SnapshotByID to return err on other cases too, and we'd treat that as happy path here.

}

snapAge := time.Now().UnixMilli() - snap.TimestampMs
Expand Down
Loading