hreddygayam commented on issue #18132: URL: https://github.com/apache/iceberg/issues/18132#issuecomment-6095662408
I'd like to pick this up — I have a working implementation and tests, but before opening a PR I want to confirm the API shape, since there's a design fork worth a committer's call. ## Approach The expensive part of `IncrementalFileCleanup.cleanFiles` is the first manifest-list scan: it reads the manifest lists of **all** retained snapshots (`afterExpiration.snapshots()`) to build the set of still-referenced manifests, so it scales with the retained-snapshot count rather than with the number of snapshots being expired. On a strictly linear history, a manifest written by an expiring snapshot that is still referenced must also be referenced by the **oldest retained snapshot**: manifests are only carried forward or dropped along a linear chain and never resurrected, so if a manifest is not in the oldest retained snapshot it cannot be in any newer one either. Scanning only that snapshot (via `SnapshotUtil.oldestAncestorOf`) is therefore sufficient, and reduces the scan from O(retained snapshots) to O(1). I scoped this to `IncrementalFileCleanup` only (not `ReachableFileCleanup`), as the proposal suggests. ## The design fork The issue proposes a strictly opt-in flag with the caller responsible for guaranteeing linearity. I implemented that — `ExpireSnapshots.optimizeForLinearHistory(boolean)` as a default method, threaded through `RemoveSnapshots` into `IncrementalFileCleanup` — **but** I also added an internal safety guard: even when the flag is set, the optimization is applied only when the retained history is actually detected to be linear (`pickedAncestorSnapshotIds` empty and every retained snapshot is an ancestor of the current snapshot); otherwise it silently falls back to the full scan. Because that guard makes the fast path safe on its own, there are two viable shapes and I'd rather you pick: 1. **Opt-in flag + guard** (what I have) — new public API `optimizeForLinearHistory(boolean)`, off by default. Matches the issue as written. 2. **Auto-detect, no public API** — drop the flag and always take the fast path when the same linearity check passes. Strictly a performance improvement with identical observable behavior and zero API surface. I lean slightly toward (2) since it needs no new API and can't be misused, but (1) is more conservative and is what the issue asked for. If (1), is `optimizeForLinearHistory` an acceptable name? ## Status Implemented against `main`; `TestRemoveSnapshots` passes (536 tests), including two new tests — a two-table equivalence test (optimized vs. full scan delete exactly the same files on linear history) and a branch-safety test (enabling the flag on a branched table never drops branch-only data). revapi and checkstyle are green. Happy to open the PR in whichever shape you prefer. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
