wombatu-kun opened a new pull request, #16645: URL: https://github.com/apache/iceberg/pull/16645
## What Adds branch support to `RewriteManifests` via `toBranch(String)` so manifest compaction/clustering can target a named branch. Closes #15981. `RewriteManifests` was the only `SnapshotUpdate` that could not commit to a branch: `BaseRewriteManifests` inherited the default `SnapshotUpdate.toBranch`, which throws `UnsupportedOperationException`. There was also a second, latent bug: even if `toBranch` were unblocked, `apply(base, snapshot)` read manifests from `base.currentSnapshot()` (always the `main` tip) rather than the `snapshot` argument that `SnapshotProducer` passes in (the tip of the target branch), so a branch commit would have rewritten the wrong manifests. ## Changes - Override `toBranch` to delegate to the inherited `SnapshotProducer.targetBranch`, which already rejects `null` and tag refs with `IllegalArgumentException`. - Read the current manifests from the `snapshot` parameter (`snapshot != null ? snapshot.allManifests(io) : Collections.emptyList()`), mirroring `FastAppend.apply` and `MergingSnapshotProducer.apply`. Rewriting an existing branch now compacts that branch's manifests without affecting `main`. Targeting a branch that does not exist yet forks it from `main` (parent = the current `main` snapshot), consistent with other `SnapshotUpdate` operations. ## Tests New tests in `TestRewriteManifests`, exercised across format versions v1-v4: - `testRewriteManifestsOnBranch`: the branch diverges from `main` (an extra file appended only on the branch) before the rewrite, so it verifies the rewrite operates on the branch tip and leaves `main` untouched. This is the case that actually exercises the `apply()` fix. - `testRewriteManifestsCreatesBranchIfNeeded`: rewriting to a non-existent branch forks it from `main`. - `testRewriteManifestsToBranchRejectsNullBranch` and `testRewriteManifestsToBranchRejectsTag`: validation. ## Notes This revives the approach from the stale PR #15982 by @jlkvanloon (added as a commit co-author), with broader test coverage. In particular, the original happy-path test appended both files to `main` before branching, so `main` and the branch were identical at rewrite time and the test did not exercise the `apply()` fix. -- 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]
