[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #5618: Support performing delete files on branches

[GitBox <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #5618:
URL: https://github.com/apache/iceberg/pull/5618#discussion_r952219398


##########
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:
   Sorry not sure what you mean by "ahead" and "behind"? Do you mean if we should validate the state of main after the branch writes? 
   
   In that case, it makes sense but I don't think its particularly necessary because the existing tests in SnapshotProducer should make sure we're committing snapshots to the correct branch in the first place meaning the existing tests would guarantee us that the operations we perform are done on the correct lineage, and existing lineages would be untouched. That being said, no harm in adding more validations! @jackye1995 @rdblue any thoughts?



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org