vbhanuchander-lang commented on PR #16645:
URL: https://github.com/apache/iceberg/pull/16645#issuecomment-5251052233
I duplicated this in #17604 before spotting yours — closed now, apologies.
Yours is the earlier and, on the manifest-content assertions, the better one.
Since I had the branch built and tested, two small additive points rather
than nothing:
**1. The null-snapshot path is fixed but untested.**
`apply` now handles `snapshot == null`, which is reachable when the table
has no snapshots at all — the previous
`base.currentSnapshot().allManifests(...)` threw `NullPointerException` there.
Worth pinning, because it is a distinct case from "branch does not exist" (that
one falls back to main via `SnapshotUtil.latestSnapshot`, which
`testRewriteManifestsCreatesBranchIfNeeded` already covers):
```java
@TestTemplate
public void testRewriteManifestsOnEmptyTable() {
assertThat(table.currentSnapshot()).isNull();
table.rewriteManifests().clusterBy(file -> "").commit();
assertThat(table.currentSnapshot().allManifests(table.io())).isEmpty();
}
```
Verified passing against your change on format versions 1–4.
**2. The `-1L` sentinel produces a confusing validation message.**
```java
validateDeletedManifests(currentManifestSet, snapshot != null ?
snapshot.snapshotId() : -1L);
```
When there is no snapshot and a caller has passed `deleteManifest`, the
failure reads:
```
Deleted manifest <path> could not be found in the latest snapshot -1
```
Passing the snapshot itself lets the message say what actually happened:
```java
private void validateDeletedManifests(Set<ManifestFile> currentManifests,
Snapshot currentSnapshot) {
deletedManifests.stream()
.filter(manifest -> !currentManifests.contains(manifest))
.findAny()
.ifPresent(
manifest -> {
if (currentSnapshot == null) {
throw new ValidationException(
"Deleted manifest %s could not be found: branch %s has no
snapshot",
manifest.path(), targetBranch());
}
throw new ValidationException(
"Deleted manifest %s could not be found in the latest
snapshot %d",
manifest.path(), currentSnapshot.snapshotId());
});
}
```
Both are optional and neither blocks the fix. Take, adapt or ignore as you
prefer — the substance of this PR is right and it has been waiting since June.
--
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]
