findinpath commented on code in PR #18408:
URL: https://github.com/apache/iceberg/pull/18408#discussion_r4219039947
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -2144,6 +2144,54 @@ public void testCleanupLevelNullValidation() {
.hasMessageContaining("Invalid cleanup level: null");
}
+ @TestTemplate
+ public void testExpireDoesNotDeletePuffinFileSharedByLiveDeletionVectors() {
+ assumeThat(formatVersion).as("Deletion vectors require V3 or
later").isGreaterThanOrEqualTo(3);
+
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+
+ // one Puffin file with deletion vectors for FILE_A and FILE_B
+ String sharedPuffinPath = "/path/to/shared-dvs-" + UUID.randomUUID() +
".puffin";
+ DeleteFile dvA = deletionVector(sharedPuffinPath, FILE_A, 4L);
+ DeleteFile dvB = deletionVector(sharedPuffinPath, FILE_B, 44L);
+ table.newRowDelta().addDeletes(dvA).addDeletes(dvB).commit();
+
Review Comment:
```
assertThat(liveDeleteFilePaths())
.as("Table should have two deletion vectors in the shared Puffin
file")
.containsExactly(sharedPuffinPath, sharedPuffinPath);
```
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -2144,6 +2144,54 @@ public void testCleanupLevelNullValidation() {
.hasMessageContaining("Invalid cleanup level: null");
}
+ @TestTemplate
+ public void testExpireDoesNotDeletePuffinFileSharedByLiveDeletionVectors() {
+ assumeThat(formatVersion).as("Deletion vectors require V3 or
later").isGreaterThanOrEqualTo(3);
+
+ table.newAppend().appendFile(FILE_A).appendFile(FILE_B).commit();
+
+ // one Puffin file with deletion vectors for FILE_A and FILE_B
+ String sharedPuffinPath = "/path/to/shared-dvs-" + UUID.randomUUID() +
".puffin";
+ DeleteFile dvA = deletionVector(sharedPuffinPath, FILE_A, 4L);
+ DeleteFile dvB = deletionVector(sharedPuffinPath, FILE_B, 44L);
+ table.newRowDelta().addDeletes(dvA).addDeletes(dvB).commit();
+
+ // replace FILE_A's deletion vector; FILE_B's stays in the shared Puffin
file
+ DeleteFile newDvA =
+ deletionVector("/path/to/new-dv-" + UUID.randomUUID() + ".puffin",
FILE_A, 4L);
+ table
+ .newRowDelta()
+ .validateFromSnapshot(table.currentSnapshot().snapshotId())
+ .removeDeletes(dvA)
+ .addDeletes(newDvA)
+ .commit();
+
+ table.newAppend().appendFile(FILE_C).commit();
+
+ long tAfterCommits =
waitUntilAfter(table.currentSnapshot().timestampMillis());
+
+ Set<String> deletedFiles = Sets.newHashSet();
+
removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
+
+ assertThat(table.snapshots()).hasSize(1);
+ assertThat(deletedFiles)
+ .as("Puffin file with a live deletion vector must not be deleted")
+ .doesNotContain(sharedPuffinPath);
+ }
Review Comment:
```
assertThat(liveDeleteFilePaths())
.as("Table should have the replacement deletion vector for FILE_A
and the shared one")
.containsExactlyInAnyOrder(newDvA.location(), dvB.location());
```
--
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]
