rdblue commented on code in PR #5618:
URL: https://github.com/apache/iceberg/pull/5618#discussion_r954273088
##########
core/src/test/java/org/apache/iceberg/TestDeleteFiles.java:
##########
@@ -300,6 +300,70 @@ public void testDeleteCaseSensitivity() {
statuses(Status.DELETED));
}
+ @Test
+ public void testMultipleFileDeletesOnBranch() {
+ String testBranch = "testBranch";
+
table.newAppend().appendFile(FILE_A).appendFile(FILE_B).appendFile(FILE_C).commit();
+ Snapshot main = readMetadata().currentSnapshot();
+
+ // Validate main history is as expected
+ Assert.assertEquals("Should have 1 manifest", 1,
main.allManifests(FILE_IO).size());
+ validateSnapshot(null, main, FILE_A, FILE_B, FILE_C);
+
+ // Perform first delete on branch
+ table.newDelete().deleteFile(FILE_A).toBranch(testBranch).commit();
+ Snapshot afterDeletingA =
table.snapshot(table.refs().get(testBranch).snapshotId());
+ Assert.assertEquals("Should have 1 manifest", 1,
afterDeletingA.allManifests(FILE_IO).size());
+ validateManifestEntries(
+ afterDeletingA.allManifests(table.io()).get(0),
+ ids(afterDeletingA.snapshotId(), main.snapshotId(), main.snapshotId()),
+ files(FILE_A, FILE_B, FILE_C),
+ statuses(Status.DELETED, Status.EXISTING, Status.EXISTING));
+ table.newDelete().deleteFile(FILE_B).toBranch(testBranch).commit();
+
+ // Perform second delete
+ Snapshot afterDeletingB =
table.snapshot(table.refs().get(testBranch).snapshotId());
+ Assert.assertEquals("Should have 1 manifest", 1,
afterDeletingB.allManifests(FILE_IO).size());
+ validateManifestEntries(
+ afterDeletingB.allManifests(FILE_IO).get(0),
+ ids(afterDeletingB.snapshotId(), main.snapshotId()),
+ files(FILE_B, FILE_C),
+ statuses(Status.DELETED, Status.EXISTING));
+ }
+
+ @Test
+ public void testDeleteWithRowFilterWithCombinedPredicatesOnBranch() {
+ String testBranch = "testBranch";
+ // add both data files
+ table
+ .newFastAppend()
+ .appendFile(DATA_FILE_BUCKET_0_IDS_0_2)
+ .appendFile(DATA_FILE_BUCKET_0_IDS_8_10)
+ .commit();
+
+ Snapshot initialSnapshot = table.currentSnapshot();
+ Assert.assertEquals("Should have 1 manifest", 1,
initialSnapshot.allManifests(FILE_IO).size());
+ validateManifestEntries(
+ initialSnapshot.allManifests(FILE_IO).get(0),
+ ids(initialSnapshot.snapshotId(), initialSnapshot.snapshotId()),
+ files(DATA_FILE_BUCKET_0_IDS_0_2, DATA_FILE_BUCKET_0_IDS_8_10),
+ statuses(Status.ADDED, Status.ADDED));
+
+ // delete the second one using a filter that relies on metrics and
partition data
+ Expression partPredicate = Expressions.equal(Expressions.bucket("data",
16), 0);
+ Expression rowPredicate = Expressions.greaterThan("id", 5);
+ Expression predicate = Expressions.and(partPredicate, rowPredicate);
+
table.newDelete().deleteFromRowFilter(predicate).toBranch(testBranch).commit();
+
+ Snapshot deleteSnapshot =
table.snapshot(table.refs().get(testBranch).snapshotId());
+ Assert.assertEquals("Should have 1 manifest", 1,
deleteSnapshot.allManifests(FILE_IO).size());
+ validateManifestEntries(
+ deleteSnapshot.allManifests(FILE_IO).get(0),
+ ids(initialSnapshot.snapshotId(), deleteSnapshot.snapshotId()),
Review Comment:
I think these tests are specific enough that we don't need to validate the
order of commits. Entries with `Status.DELETED` will only appear in a single
manifest and are dropped if the manifest is rewritten. So validating DELETE
entries can only succeed if the delete operation produced the manifest.
--
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]
