You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "paveon (via GitHub)" <gi...@apache.org> on 2026/05/22 08:53:02 UTC
[PR] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
paveon opened a new pull request, #1118:
URL: https://github.com/apache/iceberg-go/pull/1118
Fixes https://github.com/apache/iceberg-go/issues/1117
### What changed
Reworked `removeSnapshotsUpdate.PostCommit` so each unique manifest file is
opened at most once per call, regardless of how many expired or retained
snapshots reference it.
Two passes, both deduped:
1) Build the set of manifest paths reachable from any retained snapshot,
reading only manifest-lists. Cache the resulting `[]ManifestFile` per
snapshot so the retained-side pass below doesn't re-download each list.
2) Walk expired snapshots' manifest lists; for each manifest, skip if it's
in the retained set (its data files are live by definition and the
manifest itself must not be deleted) or if a prior expired snapshot
already enumerated it. Otherwise read its entries once.
3) Subtract live data files via a single walk over each unique retained
manifest. DELETED entries remain tombstones (unchanged from prior
semantics).
### Behavior
Semantically equivalent to the previous implementation — the final
`filesToDelete` set is the same on well-formed metadata. No spec change,
no API change. The only difference is the I/O cost.
### Performance impact
For a 491-snapshot incremental-append table where expiring 490 snapshots
previously triggered ~sum(1..490) ≈ 120,000 manifest-file downloads, the
rewrite reduces that to roughly the count of unique orphaned manifests
(a few hundred in practice). Two-to-three orders of magnitude fewer
object-store reads, in our test.
--
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] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
Posted by "paveon (via GitHub)" <gi...@apache.org>.
paveon commented on code in PR #1118:
URL: https://github.com/apache/iceberg-go/pull/1118#discussion_r3296287073
##########
table/updates.go:
##########
@@ -529,14 +560,17 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
- for _, snap := range postTable.Metadata().Snapshots() {
- mans, err := snap.Manifests(prefs)
- if err != nil {
- return err
- }
-
+ // Keep files still referenced (non-DELETED) by retained manifests,
+ // including data files carried forward as EXISTING by manifest
+ // merges. Each retained manifest is walked at most once.
+ walkedRetained := make(map[string]struct{}, len(retainedManifests))
Review Comment:
Good point, added the guard.
--
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] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
Posted by "paveon (via GitHub)" <gi...@apache.org>.
paveon commented on code in PR #1118:
URL: https://github.com/apache/iceberg-go/pull/1118#discussion_r3296256433
##########
table/updates.go:
##########
@@ -506,6 +506,27 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
+ // Manifest paths kept alive by retained snapshots, plus their
Review Comment:
Good point, comment changed.
--
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] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1118:
URL: https://github.com/apache/iceberg-go/pull/1118#discussion_r3288292110
##########
table/updates.go:
##########
@@ -506,6 +506,27 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
+ // Manifest paths kept alive by retained snapshots, plus their
+ // loaded manifest-file slices so the live-data-file pass below
+ // doesn't re-download each manifest list.
+ retainedManifests := make(map[string]struct{})
+ retainedSnapshotManifests := make(map[int64][]iceberg.ManifestFile)
Review Comment:
the snapshot-id key here is never read — the only consumer below ranges `for _, mans := range retainedSnapshotManifests`. I'd drop the map and either flatten while we're collecting or keep a `[][]iceberg.ManifestFile` so the walkedRetained pass below isn't paying for a map keyed on a field nobody uses.
while we're here, the inner loop already gives us each `man` once per `(snap, man)` pair — we could just dedupe by `man.FilePath()` during this pass and hand the second pass a flat `[]iceberg.ManifestFile` of unique manifests, which also removes the need for `walkedRetained` below. wdyt?
##########
table/updates.go:
##########
@@ -506,6 +506,27 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
+ // Manifest paths kept alive by retained snapshots, plus their
Review Comment:
small nit: "doesn't re-download each manifest list" reads like the whole function skips manifest-list reads, but the expired loop below still calls `snap.Manifests(prefs)` per expired snapshot. I'd tighten to something like `// Preload retained manifest lists once so the live-data-file pass below can walk them without re-fetching.` to keep the scope honest.
##########
table/updates.go:
##########
@@ -529,14 +560,17 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
- for _, snap := range postTable.Metadata().Snapshots() {
- mans, err := snap.Manifests(prefs)
- if err != nil {
- return err
- }
-
+ // Keep files still referenced (non-DELETED) by retained manifests,
+ // including data files carried forward as EXISTING by manifest
+ // merges. Each retained manifest is walked at most once.
+ walkedRetained := make(map[string]struct{}, len(retainedManifests))
Review Comment:
minor perf regression in the "nothing to delete" path: when `filesToDelete` is empty (no expired snapshots had unique manifests), this pass still opens every retained manifest and iterates entries to call `delete` on a no-op key. I'd guard the whole retained walk with `if len(filesToDelete) > 0`. Restores the short-circuit and keeps the PR's spirit of "open each manifest at most once, and only when it matters".
##########
table/updates.go:
##########
@@ -506,6 +506,27 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
+ // Manifest paths kept alive by retained snapshots, plus their
+ // loaded manifest-file slices so the live-data-file pass below
+ // doesn't re-download each manifest list.
+ retainedManifests := make(map[string]struct{})
+ retainedSnapshotManifests := make(map[int64][]iceberg.ManifestFile)
+ for _, snap := range postTable.Metadata().Snapshots() {
+ mans, err := snap.Manifests(prefs)
+ if err != nil {
+ return err
+ }
+ retainedSnapshotManifests[snap.SnapshotID] = mans
+ for _, man := range mans {
+ retainedManifests[man.FilePath()] = struct{}{}
+ }
+ }
+
+ // Open each orphaned manifest at most once: skip manifests that
+ // retained snapshots still reference, and dedupe across expired
+ // snapshots that share manifests by reference.
+ visitedManifests := make(map[string]struct{})
Review Comment:
the dedup invariant this PR is built on isn't covered by any test. `TestRemoveSnapshotsPostCommitDeletesStatisticsFiles` and `TestRemoveSnapshotsPostCommitPreservesStatisticsOfSurvivingSnapshots` both use snapshots with empty manifest lists, so `snap.Manifests()` returns `[]` and neither of the loops you're changing here ever executes.
Adding two cases against a trackingIO-style fs would lock this in: (a) manifest M shared between an expired snapshot and a retained one, asserting M and its data files survive; (b) manifest M shared between two expired snapshots, asserting M+its data files are deleted exactly once and `Entries()` is called once. The second one also pins the perf claim.
--
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] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1118:
URL: https://github.com/apache/iceberg-go/pull/1118
--
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] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
Posted by "paveon (via GitHub)" <gi...@apache.org>.
paveon commented on code in PR #1118:
URL: https://github.com/apache/iceberg-go/pull/1118#discussion_r3296286204
##########
table/updates.go:
##########
@@ -506,6 +506,27 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
+ // Manifest paths kept alive by retained snapshots, plus their
+ // loaded manifest-file slices so the live-data-file pass below
+ // doesn't re-download each manifest list.
+ retainedManifests := make(map[string]struct{})
+ retainedSnapshotManifests := make(map[int64][]iceberg.ManifestFile)
+ for _, snap := range postTable.Metadata().Snapshots() {
+ mans, err := snap.Manifests(prefs)
+ if err != nil {
+ return err
+ }
+ retainedSnapshotManifests[snap.SnapshotID] = mans
+ for _, man := range mans {
+ retainedManifests[man.FilePath()] = struct{}{}
+ }
+ }
+
+ // Open each orphaned manifest at most once: skip manifests that
+ // retained snapshots still reference, and dedupe across expired
+ // snapshots that share manifests by reference.
+ visitedManifests := make(map[string]struct{})
Review Comment:
Added two new unit tests to verify this behaviour. I also refactored the previously existing tests a little bit to reduce the duplicity of creating metadata strings over and over if that's okay.
--
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] perf(table): dedupe shared manifest reads during snapshot expiration [iceberg-go]
Posted by "paveon (via GitHub)" <gi...@apache.org>.
paveon commented on code in PR #1118:
URL: https://github.com/apache/iceberg-go/pull/1118#discussion_r3296263489
##########
table/updates.go:
##########
@@ -506,6 +506,27 @@ func (u *removeSnapshotsUpdate) PostCommit(ctx context.Context, preTable *Table,
}
}
+ // Manifest paths kept alive by retained snapshots, plus their
+ // loaded manifest-file slices so the live-data-file pass below
+ // doesn't re-download each manifest list.
+ retainedManifests := make(map[string]struct{})
+ retainedSnapshotManifests := make(map[int64][]iceberg.ManifestFile)
Review Comment:
Thanks for the suggestion, I simplified it based on your comment and went with the flat `[]iceberg.ManifestFile` slice with unique manifests.
--
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