stevenzwu commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4362986025
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1970,6 +2024,22 @@ private RemoveSnapshots removeSnapshots(Table table) {
return (RemoveSnapshots)
removeSnapshots.withIncrementalCleanup(incrementalCleanup);
}
+ // simulates a snapshot with no summary, which is valid for snapshots
written before summaries
+ // were introduced
+ private void dropSnapshotSummary(long snapshotId) throws IOException {
Review Comment:
this method name is inaccurate. it is more like `addSnapshotWithoutSummary`.
But for this trivial null check, I am not sure we need to add unit tests.
does it impact the cleanup behavior or we just want to tolerate null summary?
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -97,11 +97,9 @@ public void cleanFiles(
Set<Long> pickedAncestorSnapshotIds = Sets.newHashSet();
for (long snapshotId : ancestorIds) {
+ Map<String, String> summary =
beforeExpiration.snapshot(snapshotId).summary();
String sourceSnapshotId =
- beforeExpiration
- .snapshot(snapshotId)
- .summary()
- .get(SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP);
+ summary != null ?
summary.get(SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP) : null;
Review Comment:
Use `PropertyUtil` here too, same as the expired-snapshot check below. `get`
plus `Long.parseLong` is a second way to read this property.
```java
Long sourceSnapshotId =
summary != null
? PropertyUtil.propertyAsNullableLong(summary,
SnapshotSummary.SOURCE_SNAPSHOT_ID_PROP)
: null;
if (sourceSnapshotId != null) {
pickedAncestorSnapshotIds.add(sourceSnapshotId);
}
```
--
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]
