Implement snapshot expiration #401

arnaudbriche · 2025-04-22T13:09:42Z

As discussed on a previous PR, I'm trying to implement snapshots expiration now.
This is not complete, but I wanted to discuss it with you before I further work on it.

If the design looks ok to you, here are a few technical questions regarding snapshot expiration:

currently, the snapshots field of the produced metadata file looks ok (expired snapshots are not there), but the snapshot-log field still contains entries for every operation since table creation. Is is expected ? I don't find an appropriate answer in the spec.
should we handle expired data and metadata files deletion here ?
the produced metadata file does not contains any new snapshot (not snapshot is created for the expire snapshots operation). I cannot find out what the spec says about it. Most probably a new snapshot must be created.

References

https://iceberg.apache.org/spec/#snapshot-retention-policy

zeroshade · 2025-05-01T22:01:15Z

table/transaction.go

+	}
+
+	for refName, ref := range t.meta.refs {


can we combine these loops and just put the continue after refsToDelete[refName] = struct{}{}?

I tried to keep it as explained here: https://iceberg.apache.org/spec/#snapshot-retention-policy
I can change that if you think it's better.

I don't see anything in that description which prevents combining these loops into a single loop, so let's change it to simplify the logic and avoid the extra trip through all of the refs.

zeroshade

Looks good! other than a few comments, the only thing missing here is some tests. Can you add some tests please?

arnaudbriche · 2025-05-02T12:52:04Z

Hi @zeroshade

Sure, I'll add tests.
Regarding these 3 points, can you provide guidance/opinions ?

currently, the snapshots field of the produced metadata file looks ok (expired snapshots are not there), but the snapshot-log field still contains entries for every operation since table creation. Is is expected ? I don't find an appropriate answer in the spec.
should we handle expired data and metadata files deletion here ?
the produced metadata file does not contains any new snapshot (not snapshot is created for the expire snapshots operation). I cannot find out what the spec says about it. Most probably a new snapshot must be created.

If we decide to handle file removal as part of ExpireSnapshots, we would have to postpone the actual deletion after the commit is successful. I don't see any easy way to do it.

zeroshade · 2025-05-02T15:27:47Z

@kevinjqliu @Fokko do either of you know the answers to @arnaudbriche's questions here you both have more experience with the spec side of things than I do as far as what makes sense for those situations.

arnaudbriche · 2025-05-02T15:59:22Z

I just added a test and implemented snapshot-log pruning because it's growing unbounded otherwise.

arnaudbriche · 2025-05-02T16:03:32Z

Regarding file deletion, maybe we can add a new method to the Update interface where we can put some post-commit code.
What do you think @zeroshade ?

table/metadata.go

table/transaction.go

zeroshade · 2025-05-06T19:51:55Z

Regarding file deletion, maybe we can add a new method to the Update interface where we can put some post-commit code.

It's a good question. If we have the table, then we have the file system interface so that we can remove the files. It might make sense for us to simply have an option to tell it whether or not to delete the files (rather than allow generic post-commit code).

kevinjqliu · 2025-05-09T17:17:42Z

currently, the snapshots field of the produced metadata file looks ok (expired snapshots are not there), but the snapshot-log field still contains entries for every operation since table creation. Is is expected ? I don't find an appropriate answer in the spec.

the snapshot-log should be cleaned up as part of expiration. From https://iceberg.apache.org/spec/#table-metadata-fields, in the snapshot-log entry of the table.
"""
A list (optional) of timestamp and snapshot ID pairs that encodes changes to the current snapshot for the table. Each time the current-snapshot-id is changed, a new entry should be added with the last-updated-ms and the new current-snapshot-id. When snapshots are expired from the list of valid snapshots, all entries before a snapshot that has expired should be removed.
"""

should we handle expired data and metadata files deletion here ?

From the ExpireSnapshots javadoc, it looks you can optionally remove the data and metadata files, but its set to true by default

the produced metadata file does not contains any new snapshot (not snapshot is created for the expire snapshots operation). I cannot find out what the spec says about it. Most probably a new snapshot must be created.

Also from the java docs for ExpireSnapshots,
"""
This API accumulates snapshot deletions and commits the new list to the table. This API does not allow deleting the current snapshot.

When committing, these changes will be applied to the latest table metadata. Commit conflicts will be resolved by applying the changes to the new latest metadata and reattempting the commit.
"""

A new snapshot should be created reflecting the newest table update, i.e. old snapshots removed

Hope this helps! Happy to help point to any other questions :)

arnaudbriche · 2025-05-14T15:58:01Z

A new snapshot should be created reflecting the newest table update, i.e. old snapshots removed

Thx for all the answers @kevinjqliu.
I implemented unreachable files deletion and snapshot log pruning as specified in your reply.

I have a hard time figuring out how a new snapshot could be created, I we technically don't write , update or delete any data from the table here.
Manifest would be exactly the same as current snapshot's one.

…e slice's underlying arrays with the old one

zeroshade · 2025-05-19T15:44:25Z

table/metadata.go

-	b.schemaList = metadata.Schemas()
+	b.schemaList = slices.Clone(metadata.Schemas())
 	b.currentSchemaID = metadata.CurrentSchema().ID
-	b.specs = metadata.PartitionSpecs()
+	b.specs = slices.Clone(metadata.PartitionSpecs())
 	b.defaultSpecID = metadata.DefaultPartitionSpec()
 	b.lastPartitionID = metadata.LastPartitionSpecID()
-	b.props = metadata.Properties()
-	b.snapshotList = metadata.Snapshots()
-	b.sortOrderList = metadata.SortOrders()
+	b.props = maps.Clone(metadata.Properties())
+	b.snapshotList = slices.Clone(metadata.Snapshots())
+	b.sortOrderList = slices.Clone(metadata.SortOrders())


I wonder if there's a way we could lazily do this cloning? Avoid performing the copies until we actually need to?

zeroshade · 2025-05-19T15:50:00Z

table/transaction.go

+		refAge := time.Now().UnixMilli() - snap.TimestampMs
+		if refAge > *maxRefAgeMs {
+			updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+			refsToDelete[refName] = struct{}{}
+		}


extract time.Now().UnixMilli() from the loop instead of calling it repeatedly. Stick it in the var block at the top of this function.

zeroshade · 2025-05-19T15:57:19Z

table/updates.go

+	for _, snap := range preTable.Metadata().Snapshots() {
+		if slices.Contains(u.SnapshotIDs, snap.SnapshotID) {
+			filesToDelete[snap.ManifestList] = struct{}{}
+		}
+	}


We have our list of snapshotIDs, so why do we need this loop?

zeroshade · 2025-05-19T16:06:23Z

table/updates.go

+		mans, err := snap.Manifests(preTable.fs)
+		if err != nil {
+			return err
+		}
+
+		for _, man := range mans {
+			filesToDelete[man.FilePath()] = struct{}{}
+
+			entries, err := man.FetchEntries(preTable.fs, false)
+			if err != nil {
+				return err
+			}
+
+			for _, entry := range entries {
+				filesToDelete[entry.DataFile().FilePath()] = struct{}{}
+			}
+		}


Snapshot objects have a dataFiles method which will return an iterator over the files, maybe we can leverage this to encapsulate this logic more rather than duplicate it?

zeroshade

This is shaping up nicely! Just a few questions left.

laskoviymishka · 2025-06-04T15:09:47Z

internal/coalesce.go

+
+package internal
+
+func Coalesce[T any](vals ...*T) *T {


nit: better to move it to utils.go, no need to span a new file just for this function.

zeroshade reviewed May 1, 2025

View reviewed changes

arnaudbriche force-pushed the expire-snapshots branch from a4b4b36 to ec8d33b Compare May 2, 2025 15:56

zeroshade requested changes May 6, 2025

View reviewed changes

table/metadata.go Outdated Show resolved Hide resolved

table/transaction.go Show resolved Hide resolved

arnaudbriche added 4 commits May 14, 2025 17:47

Basic version of expireSnapshots operation.

3e2f68f

Fix sad linter

b32a4dd

Add snapshot-log pruning and a test

71061c1

Implement unreachable files deletion in ExpireSnapshots

97a5820

arnaudbriche force-pushed the expire-snapshots branch from ec8d33b to 97a5820 Compare May 14, 2025 15:48

arnaudbriche changed the title ~~Basic version of expireSnapshots operation.~~ Implement snapshot expiration May 14, 2025

arnaudbriche added 2 commits May 14, 2025 18:12

Simplify RemoveSnapshots

233586b

Modify MetadataBuilderFromBase so that the new Metadata does not shar…

b860a6a

…e slice's underlying arrays with the old one

arnaudbriche requested a review from zeroshade May 18, 2025 18:55

zeroshade reviewed May 19, 2025

View reviewed changes

zeroshade requested changes May 19, 2025

View reviewed changes

laskoviymishka reviewed Jun 4, 2025

View reviewed changes

Implement snapshot expiration #401

Are you sure you want to change the base?

Implement snapshot expiration #401

Uh oh!

Conversation

arnaudbriche commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Uh oh!

zeroshade May 1, 2025

Choose a reason for hiding this comment

Uh oh!

arnaudbriche May 14, 2025

Choose a reason for hiding this comment

Uh oh!

zeroshade May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

arnaudbriche commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade commented May 2, 2025

Uh oh!

arnaudbriche commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnaudbriche commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeroshade commented May 6, 2025

Uh oh!

kevinjqliu commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnaudbriche commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade May 19, 2025

Choose a reason for hiding this comment

Uh oh!

zeroshade May 19, 2025

Choose a reason for hiding this comment

Uh oh!

zeroshade May 19, 2025

Choose a reason for hiding this comment

Uh oh!

zeroshade May 19, 2025

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

laskoviymishka Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arnaudbriche commented Apr 22, 2025 •

edited

Loading

zeroshade May 19, 2025 •

edited

Loading

arnaudbriche commented May 2, 2025 •

edited

Loading

arnaudbriche commented May 2, 2025 •

edited

Loading

arnaudbriche commented May 2, 2025 •

edited

Loading

kevinjqliu commented May 9, 2025 •

edited

Loading

arnaudbriche commented May 14, 2025 •

edited

Loading