vbhanuchander-lang opened a new pull request, #17604:
URL: https://github.com/apache/iceberg/pull/17604
Closes #15981
Builds on @jlkvanloon's earlier attempt in #15982, which was closed by the
stale bot without ever getting a review. Credit for the diagnosis is theirs —
both defects below are described in the issue.
### Two defects, not one
**1. `toBranch` was never overridden.** `RewriteManifests` inherited the
default `SnapshotUpdate.toBranch`, which throws, so
`table.rewriteManifests().toBranch("b").commit()` failed with
`UnsupportedOperationException`.
**2. `apply` read the wrong snapshot.** This is the part that matters, and
unblocking `toBranch` without it would have been worse than the exception:
```java
List<ManifestFile> currentManifests =
base.currentSnapshot().allManifests(ops().io());
```
`SnapshotProducer` calls `apply(base, parentSnapshot)` where `parentSnapshot
= SnapshotUtil.latestSnapshot(base, targetBranch)` — the tip of the branch
being committed to. Reading `base.currentSnapshot()` instead means a rewrite
aimed at a branch would compact **main's** manifests and commit the result to
the branch. Silently the wrong data, no error.
### The change
- `toBranch` delegates to `targetBranch`, matching `FastAppend`,
`MergeAppend`, `BaseRowDelta` and the rest. Null and tag-name validation
already lives in `targetBranch`, so the behaviour the issue asks for comes free
rather than being re-implemented.
- `apply` reads the `snapshot` parameter, treating null as an empty manifest
list. Null happens when the table has no snapshots at all — where the previous
code threw `NullPointerException`.
- `validateDeletedManifests` takes the snapshot rather than a snapshot id,
so its message is accurate when there is no snapshot to name.
### One behaviour worth confirming
Targeting a branch that does not exist **creates it from main's current
snapshot**, because `SnapshotUtil.latestSnapshot` falls back to
`metadata.currentSnapshot()` for an absent ref:
```java
SnapshotRef ref = metadata.ref(branch);
if (ref == null) {
return metadata.currentSnapshot();
}
```
That is consistent with `AppendFiles` — appending to a new branch also
starts from main. I wrote the test expecting an empty branch first, it failed,
and I traced it here rather than changing the assertion to match. Flagging it
explicitly in case you would rather a missing branch were rejected.
### Tests
`testRewriteManifestsOnBranchUnsupported` is replaced by coverage of each
behaviour the operation now has. All run across format versions 1–4:
| Test | Asserts |
|---|---|
| `testRewriteManifestsOnBranch` | rewriting a branch produces a `REPLACE`
snapshot on that branch, main does not move |
| `testRewriteManifestsOnBranchUsesBranchManifestsNotMain` | main has 1
file, branch has 3; the rewrite carries 3 — this is the test that fails against
the old `apply` |
| `testRewriteManifestsCreatesMissingBranchFromMain` | absent branch is
created from main, main untouched |
| `testRewriteManifestsRejectsInvalidBranch` | null and tag targets throw
`IllegalArgumentException` |
| `testRewriteManifestsOnEmptyTable` | no snapshots no longer throws
`NullPointerException` |
```
TestRewriteManifests: tests=136 failures=0 errors=0
```
`:iceberg-core:checkstyleMain`, `:iceberg-core:checkstyleTest` and
`:iceberg-core:spotlessCheck` pass. The `*RewriteManifests*`,
`*SnapshotProducer*` and `*Branch*` core suites pass, and nothing else in the
tree asserted the old unsupported-branch behaviour.
### Why this is wanted
From the issue: manifest clustering (`clusterBy(DataFile::partition)`) on a
branch before merging it into main, so a write-audit-publish pipeline can
compact branch metadata rather than merging unoptimised metadata into main and
compacting afterwards.
--
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]
