You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "arnaudbriche (via GitHub)" <gi...@apache.org> on 2025/04/22 13:09:45 UTC
[PR] Basic version of expireSnapshots operation. [iceberg-go]
arnaudbriche opened a new pull request, #401:
URL: https://github.com/apache/iceberg-go/pull/401
Hi @zeroshade,
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2096071405
##########
table/updates.go:
##########
@@ -382,7 +389,74 @@ func NewRemoveSnapshotsUpdate(ids []int64) Update {
}
func (u *removeSnapshotsUpdate) Apply(builder *MetadataBuilder) error {
- return fmt.Errorf("%w: %s", iceberg.ErrNotImplemented, UpdateRemoveSnapshots)
+ _, err := builder.RemoveSnapshots(u.SnapshotIDs)
+
+ return err
+}
+
+func (u *removeSnapshotsUpdate) PostCommit(preTable *Table, postTable *Table) error {
+ filesToDelete := make(map[string]struct{})
+
+ for _, snap := range preTable.Metadata().Snapshots() {
+ if slices.Contains(u.SnapshotIDs, snap.SnapshotID) {
+ filesToDelete[snap.ManifestList] = struct{}{}
+ }
+ }
+
+ for _, snapId := range u.SnapshotIDs {
+ snap := preTable.SnapshotByID(snapId)
+ if snap == nil {
+ return errors.New("missing snapshot")
+ }
+
+ 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{}{}
+ }
+ }
Review Comment:
`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?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-3001933822
@arnaudbriche any updates?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-2867338414
> 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](https://iceberg.apache.org/javadoc/1.9.0/org/apache/iceberg/ExpireSnapshots.html) javadoc, it looks you can optionally remove the data and metadata files, but its [set to false by default](https://github.com/apache/iceberg/blob/a5bcacd979dc9ac70be3d7e5b93bb967ff04f71a/core/src/main/java/org/apache/iceberg/RemoveSnapshots.java#L72)
> 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`](https://iceberg.apache.org/javadoc/1.9.0/org/apache/iceberg/ExpireSnapshots.html),
"""
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 :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-2855771394
> 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).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2096041216
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
+
+ refAge := time.Now().UnixMilli() - snap.TimestampMs
+ if refAge > *maxRefAgeMs {
+ updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+ refsToDelete[refName] = struct{}{}
+ }
+ }
+
+ for refName, ref := range t.meta.refs {
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2096028620
##########
table/metadata.go:
##########
@@ -187,14 +187,14 @@ func MetadataBuilderFromBase(metadata Metadata) (*MetadataBuilder, error) {
b.loc = metadata.Location()
b.lastUpdatedMS = metadata.LastUpdatedMillis()
b.lastColumnId = metadata.LastColumnID()
- 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())
Review Comment:
I wonder if there's a way we could lazily do this cloning? Avoid performing the copies until we actually need to?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "arnaudbriche (via GitHub)" <gi...@apache.org>.
arnaudbriche commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-2847133044
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "arnaudbriche (via GitHub)" <gi...@apache.org>.
arnaudbriche commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-2847577978
I just add a test and implemented snapshot-log pruning because it's growing unbounded otherwise.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-3097251481
@arnaudbriche looks like one of the tests are failing:
```
=== RUN TestTableWriting/TestExpireSnapshots
table_test.go:933:
Error Trace: /home/runner/work/iceberg-go/iceberg-go/table/table_test.go:933
Error: Received unexpected error:
cannot remove main branch's snapshot ref
Test: TestTableWriting/TestExpireSnapshots
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2126849521
##########
internal/coalesce.go:
##########
@@ -0,0 +1,28 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package internal
+
+func Coalesce[T any](vals ...*T) *T {
Review Comment:
nit: better to move it to [utils.go](https://github.com/apache/iceberg-go/blob/main/internal/utils.go), no need to span a new file just for this function.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2096050696
##########
table/updates.go:
##########
@@ -382,7 +389,74 @@ func NewRemoveSnapshotsUpdate(ids []int64) Update {
}
func (u *removeSnapshotsUpdate) Apply(builder *MetadataBuilder) error {
- return fmt.Errorf("%w: %s", iceberg.ErrNotImplemented, UpdateRemoveSnapshots)
+ _, err := builder.RemoveSnapshots(u.SnapshotIDs)
+
+ return err
+}
+
+func (u *removeSnapshotsUpdate) PostCommit(preTable *Table, postTable *Table) error {
+ filesToDelete := make(map[string]struct{})
+
+ for _, snap := range preTable.Metadata().Snapshots() {
+ if slices.Contains(u.SnapshotIDs, snap.SnapshotID) {
+ filesToDelete[snap.ManifestList] = struct{}{}
+ }
+ }
Review Comment:
We have our list of snapshotIDs, so why do we need this loop?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "arnaudbriche (via GitHub)" <gi...@apache.org>.
arnaudbriche commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2174396326
##########
table/transaction.go:
##########
@@ -142,6 +143,121 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ nowMs = time.Now().UnixMilli()
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
Review Comment:
Indeed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "arnaudbriche (via GitHub)" <gi...@apache.org>.
arnaudbriche commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-2847585914
Regarding file deletion, maybe we can add a new method to the `Update` interface where we can put some post-commit code.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2096041216
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
+
+ refAge := time.Now().UnixMilli() - snap.TimestampMs
+ if refAge > *maxRefAgeMs {
+ updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+ refsToDelete[refName] = struct{}{}
+ }
+ }
+
+ for refName, ref := range t.meta.refs {
Review Comment:
I don't see anything in that description which prevents combining these loops into a single loop
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #401:
URL: https://github.com/apache/iceberg-go/pull/401
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2096038254
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
+
+ refAge := time.Now().UnixMilli() - snap.TimestampMs
+ if refAge > *maxRefAgeMs {
+ updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+ refsToDelete[refName] = struct{}{}
+ }
Review Comment:
extract `time.Now().UnixMilli()` from the loop instead of calling it repeatedly. Stick it in the var block at the top of this function.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2070861384
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
+
+ refAge := time.Now().UnixMilli() - snap.TimestampMs
+ if refAge > *maxRefAgeMs {
+ updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+ refsToDelete[refName] = struct{}{}
+ }
+ }
+
+ for refName, ref := range t.meta.refs {
Review Comment:
can we combine these loops and just put the continue after `refsToDelete[refName] = struct{}{}`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2175426593
##########
table/updates.go:
##########
@@ -382,7 +390,85 @@ func NewRemoveSnapshotsUpdate(ids []int64) Update {
}
func (u *removeSnapshotsUpdate) Apply(builder *MetadataBuilder) error {
- return fmt.Errorf("%w: %s", iceberg.ErrNotImplemented, UpdateRemoveSnapshots)
+ _, err := builder.RemoveSnapshots(u.SnapshotIDs)
+
+ return err
+}
+
+func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table, postTable *Table) error {
+ prefs, err := preTable.FS(ctx)
+ if err != nil {
+ return err
+ }
+
+ filesToDelete := make(map[string]struct{})
+
+ for _, snapId := range u.SnapshotIDs {
+ snap := preTable.Metadata().SnapshotByID(snapId)
+ if snap == nil {
+ return errors.New("snapshot should never be nil")
+ }
+
+ filesToDelete[snap.ManifestList] = struct{}{}
+ }
+
+ for _, snapId := range u.SnapshotIDs {
+ snap := preTable.SnapshotByID(snapId)
+ if snap == nil {
+ return errors.New("missing snapshot")
+ }
+
+ mans, err := snap.Manifests(prefs)
+ if err != nil {
+ return err
+ }
+
+ for _, man := range mans {
+ filesToDelete[man.FilePath()] = struct{}{}
+
+ entries, err := man.FetchEntries(prefs, false)
+ if err != nil {
+ return err
+ }
+
+ for _, entry := range entries {
+ filesToDelete[entry.DataFile().FilePath()] = struct{}{}
+ }
+ }
+ }
+
+ for _, snap := range postTable.Metadata().Snapshots() {
+ mans, err := snap.Manifests(prefs)
+ if err != nil {
+ return err
+ }
+
+ for _, man := range mans {
+ delete(filesToDelete, man.FilePath())
+
+ entries, err := man.FetchEntries(prefs, false)
+ if err != nil {
+ return err
+ }
+
+ for _, entry := range entries {
+ if entry.Status() != iceberg.EntryStatusDELETED {
+ delete(filesToDelete, entry.DataFile().FilePath())
+ }
+ }
+ }
+ }
+
+ var res error
+
+ for f := range filesToDelete {
+ if err := prefs.Remove(f); err != nil {
+ res = errors.Join(res, err)
+ }
+ fmt.Println("remove", f)
Review Comment:
this should be a log line or something, but either way should not be printing to the console.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "arnaudbriche (via GitHub)" <gi...@apache.org>.
arnaudbriche commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2175852857
##########
table/updates.go:
##########
@@ -382,7 +390,85 @@ func NewRemoveSnapshotsUpdate(ids []int64) Update {
}
func (u *removeSnapshotsUpdate) Apply(builder *MetadataBuilder) error {
- return fmt.Errorf("%w: %s", iceberg.ErrNotImplemented, UpdateRemoveSnapshots)
+ _, err := builder.RemoveSnapshots(u.SnapshotIDs)
+
+ return err
+}
+
+func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table, postTable *Table) error {
+ prefs, err := preTable.FS(ctx)
+ if err != nil {
+ return err
+ }
+
+ filesToDelete := make(map[string]struct{})
+
+ for _, snapId := range u.SnapshotIDs {
+ snap := preTable.Metadata().SnapshotByID(snapId)
+ if snap == nil {
+ return errors.New("snapshot should never be nil")
+ }
+
+ filesToDelete[snap.ManifestList] = struct{}{}
+ }
+
+ for _, snapId := range u.SnapshotIDs {
+ snap := preTable.SnapshotByID(snapId)
+ if snap == nil {
+ return errors.New("missing snapshot")
+ }
+
+ mans, err := snap.Manifests(prefs)
+ if err != nil {
+ return err
+ }
+
+ for _, man := range mans {
+ filesToDelete[man.FilePath()] = struct{}{}
+
+ entries, err := man.FetchEntries(prefs, false)
+ if err != nil {
+ return err
+ }
+
+ for _, entry := range entries {
+ filesToDelete[entry.DataFile().FilePath()] = struct{}{}
+ }
+ }
+ }
+
+ for _, snap := range postTable.Metadata().Snapshots() {
+ mans, err := snap.Manifests(prefs)
+ if err != nil {
+ return err
+ }
+
+ for _, man := range mans {
+ delete(filesToDelete, man.FilePath())
+
+ entries, err := man.FetchEntries(prefs, false)
+ if err != nil {
+ return err
+ }
+
+ for _, entry := range entries {
+ if entry.Status() != iceberg.EntryStatusDELETED {
+ delete(filesToDelete, entry.DataFile().FilePath())
+ }
+ }
+ }
+ }
+
+ var res error
+
+ for f := range filesToDelete {
+ if err := prefs.Remove(f); err != nil {
+ res = errors.Join(res, err)
+ }
+ fmt.Println("remove", f)
Review Comment:
Yep. I just forgot to remove it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2180249589
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
+
+ refAge := time.Now().UnixMilli() - snap.TimestampMs
+ if refAge > *maxRefAgeMs {
+ updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+ refsToDelete[refName] = struct{}{}
+ }
+ }
+
+ for refName, ref := range t.meta.refs {
Review Comment:
I think this comment to combine these loops is the only comment I have left @arnaudbriche
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2076149034
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
Review Comment:
we can't remove snapshots from the main branch?
##########
table/metadata.go:
##########
@@ -335,6 +335,36 @@ func (b *MetadataBuilder) AddSnapshot(snapshot *Snapshot) (*MetadataBuilder, err
return b, nil
}
+func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64) (*MetadataBuilder, error) {
+ var snapshotsToKeep []Snapshot
+
+ for _, snapshot := range b.snapshotList {
+ if slices.Contains(snapshotIds, snapshot.SnapshotID) {
+ if snapshot.SnapshotID == *b.currentSnapshotID {
+ return nil, errors.New("current snapshot cannot be removed")
+ }
+
+ continue
+ }
+
+ snapshotsToKeep = append(snapshotsToKeep, snapshot)
+ }
+
+ var prunedSnapshotLog []SnapshotLogEntry
+
+ for _, entry := range b.snapshotLog {
+ if !slices.Contains(snapshotIds, entry.SnapshotID) {
+ prunedSnapshotLog = append(prunedSnapshotLog, entry)
+ }
+ }
Review Comment:
Maybe we can simplify this a bit?
```go
if slices.Contains(snapshotIds, *b.currentSnapshotID) {
return nil, errors.New("current snapshot cannot be removed")
}
b.snapshotList = slices.DeleteFunc(b.snapshotList, func(e Snapshot) bool {
return slices.Contains(snapshotIds, e.SnapshotID) })
b.snapshotLog = slices.DeleteFunc(b.snapshotLog, func(e SnapshotLogEntry) bool {
return slices.Contains(snapshotIds, e.SnapshotID) })
b.updates = append(b.updates, NewRemoveSnapshotsUpdate(snapshotIds))
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): Implement snapshot expiration [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#discussion_r2175434203
##########
table/transaction.go:
##########
@@ -142,6 +143,120 @@ func (t *Transaction) SetProperties(props iceberg.Properties) error {
return nil
}
+type expireSnapshotsCfg struct {
+ minSnapshotsToKeep *int
+ maxSnapshotAgeMs *int64
+}
+
+type ExpireSnapshotsOpt func(*expireSnapshotsCfg)
+
+func WithRetainLast(n int) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ cfg.minSnapshotsToKeep = &n
+ }
+}
+
+func WithOlderThan(t time.Duration) ExpireSnapshotsOpt {
+ return func(cfg *expireSnapshotsCfg) {
+ n := t.Milliseconds()
+ cfg.maxSnapshotAgeMs = &n
+ }
+}
+
+func (t *Transaction) ExpireSnapshots(opts ...ExpireSnapshotsOpt) error {
+ var (
+ cfg expireSnapshotsCfg
+ updates []Update
+ snapsToKeep = make(map[int64]struct{})
+ refsToDelete = make(map[string]struct{})
+ )
+
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ for refName, ref := range t.meta.refs {
+ if refName == MainBranch {
+ continue
+ }
+
+ snap, err := t.meta.SnapshotByID(ref.SnapshotID)
+ if err != nil {
+ return err
+ }
+
+ maxRefAgeMs := internal.Coalesce(ref.MaxRefAgeMs, cfg.maxSnapshotAgeMs)
+ if maxRefAgeMs == nil {
+ return errors.New("cannot find a valid value for maxRefAgeMs")
+ }
+
+ refAge := time.Now().UnixMilli() - snap.TimestampMs
+ if refAge > *maxRefAgeMs {
+ updates = append(updates, NewRemoveSnapshotRefUpdate(refName))
+ refsToDelete[refName] = struct{}{}
+ }
+ }
+
+ for refName, ref := range t.meta.refs {
Review Comment:
I still think it would be beneficial to combine these loops
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Basic version of expireSnapshots operation. [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #401:
URL: https://github.com/apache/iceberg-go/pull/401#issuecomment-2847495468
@kevinjqliu @Fokko do either of you know the answers to @arnaudbriche's questions [here](https://github.com/apache/iceberg-go/pull/401#issuecomment-2847133044) you both have more experience with the spec side of things than I do as far as what makes sense for those situations.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org