xanderbailey opened a new pull request, #17929: URL: https://github.com/apache/iceberg/pull/17929
### Summary `ExpireSnapshots.CleanupLevel` was added to the core API in #14287 but was never exposed through any engine. This PR plumbs it through the Spark action and the `expire_snapshots` procedure. Opening this as a draft to have something concrete to discuss on the dev list — see the open questions below. ### Why it isn't a pass-through `ExpireSnapshotsSparkAction` already calls `cleanupLevel(CleanupLevel.NONE)` on the core operation and derives the files to delete itself, by anti-joining the file lists before and after expiration. Passing the user's level down to core would therefore have no effect at all. Instead, the level is applied to that diff: | Level | Spark behaviour | | --- | --- | | `ALL` (default) | unchanged — deletes metadata and content files | | `METADATA_ONLY` | deletes manifests, manifest lists and statistics files; retains content files | | `NONE` | commits the expiration, deletes nothing | `METADATA_ONLY` drops `contentFileDS` from both sides of the anti-join rather than filtering the result, so the manifests of expired snapshots are never opened. That mirrors the original motivation in #14287, where reading manifests — not the deletes themselves — was identified as the bottleneck. ### Changes - `actions.ExpireSnapshots`: new `cleanupLevel(CleanupLevel)` default method that throws `UnsupportedOperationException`, matching `cleanExpiredMetadata`. - `ExpireSnapshots.CleanupLevel`: new `fromString`, mirroring `DeleteOrphanFiles.PrefixMismatchMode.fromString`, so engines can accept the level as a string. - `ExpireSnapshotsSparkAction`: implements `cleanupLevel`; reports the level in the job description when it is not the default. - `expire_snapshots` procedure: new optional `cleanup_level` string parameter. - Tests for both non-default levels through the action and the procedure, plus invalid input; docs for the new parameter. `revapi` is clean — the only API additions are a default method and a static method on an enum. ### Open questions for discussion 1. **Does `NONE` belong on the Spark action?** The action exists to distribute deletion, so `NONE` reduces it to a local metadata commit that `table.expireSnapshots().cleanupLevel(NONE)` already does. It is included here because the procedure is the only way a SQL user can express "expire without deleting", but restricting the action to `ALL`/`METADATA_ONLY` is a reasonable alternative. 2. **Parameter shape.** `cleanup_level` as a string enum follows `prefix_mismatch_mode` on `remove_orphan_files`. A boolean pair was the alternative but does not extend if more levels are added. 3. **Interaction with `gc.enabled`.** The action still rejects GC-disabled tables in its constructor, so `METADATA_ONLY` and `NONE` are unreachable on exactly the tables that most want them (Nessie, `snapshot`, `snapshotDeltaLakeTable`). Relaxing that to mirror core — reject only the file-deleting levels — depends on #17791 and is deliberately left out of this PR. Should that land here as a follow-up, or as part of #17791? 4. **Scope.** Spark 4.1 only, to keep the diff focused on the API shape. Backports to 4.0 and 3.5 are mechanical. Flink's `ExpireSnapshotsProcessor` hardcodes `CleanupLevel.ALL` and would need a builder option; happy to do either in this PR or separately. 🤖 Generated with [Claude Code](https://claude.com/claude-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: [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]
