stevenzwu commented on code in PR #18624:
URL: https://github.com/apache/iceberg/pull/18624#discussion_r4391642420
##########
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 addSnapshotWithoutSummary(long snapshotId) throws IOException {
Review Comment:
use JSON serialization round-trip to modify an existing snapshot seems hacky.
why don't we construct the Snapshot and TableMetadata directly via
constructor or builder?
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -110,6 +113,57 @@ public void testExpireOlderThan() {
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
}
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnSurvivingAncestor() throws
IOException {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ addSnapshotWithoutSummary(secondSnapshot.snapshotId());
+ assertThat(table.snapshot(secondSnapshot.snapshotId()).summary()).isNull();
+
+ long tAfterSecondCommit =
waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
+ removeSnapshots(table)
+ .expireOlderThan(tAfterSecondCommit)
+ .deleteWith(deletedFiles::add)
+ .commit();
+
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNotNull();
+
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
+ }
+
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnExpiredAncestor() throws
IOException {
Review Comment:
how is this test different with the one above?
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -110,6 +113,57 @@ public void testExpireOlderThan() {
assertThat(deletedFiles).containsExactly(firstSnapshot.manifestListLocation());
}
+ @TestTemplate
+ public void expireOlderThanWithNullSummaryOnSurvivingAncestor() throws
IOException {
+ assumeThat(incrementalCleanup).isTrue();
+
+ table.newAppend().appendFile(FILE_A).commit();
+ Snapshot firstSnapshot = table.currentSnapshot();
+
+ waitUntilAfter(firstSnapshot.timestampMillis());
+
+ table.newAppend().appendFile(FILE_B).commit();
+ Snapshot secondSnapshot = table.currentSnapshot();
+ addSnapshotWithoutSummary(secondSnapshot.snapshotId());
+ assertThat(table.snapshot(secondSnapshot.snapshotId()).summary()).isNull();
+
+ long tAfterSecondCommit =
waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
+ removeSnapshots(table)
+ .expireOlderThan(tAfterSecondCommit)
Review Comment:
this condition is a little weird. by time, the second snapshot should be
expired. but I guess the second snapshot is retained because it is the only
snapshot retained. is this the scenario we are targeting? if not, why don't
expire a timestamp after the first snapshot?
##########
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:
I would drop both tests and `addSnapshotWithoutSummary`.
The NPE stopped expiration, but a null summary does not change which files
are deleted. Cleanup reads the summary only to find `source-snapshot-id`. With
no summary, that id is missing, so the cherry-pick checks do not apply and
expiration deletes the same files as a normal append. `testExpireOlderThan`
already checks that. Both new tests check it again: the older snapshot is
removed, and only its manifest list is deleted.
A test whose only new check is "expiration does not throw" needs a metadata
rewrite to strip the summary. That setup is more than this null check needs.
--
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]
