dramaticlly commented on code in PR #10935:
URL: https://github.com/apache/iceberg/pull/10935#discussion_r1767264496
##########
core/src/test/java/org/apache/iceberg/TestBaseIncrementalChangelogScan.java:
##########
@@ -132,6 +131,175 @@ public void testFileDeletes() {
assertThat(t1.existingDeletes()).as("Must be no deletes").isEmpty();
}
+ @TestTemplate
+ public void testRowDeletes() {
+ assumeThat(formatVersion).isEqualTo(2);
+
+ table
+ .newFastAppend()
+ .appendFile(FILE_A)
+ .appendFile(FILE_A2)
+ .appendFile(FILE_B)
+ .appendFile(FILE_C)
+ .commit();
+ Snapshot snap1 = table.currentSnapshot();
+
+ // position delete
+ table.newRowDelta().addDeletes(FILE_B_DELETES).commit();
+ Snapshot snap2 = table.currentSnapshot();
+
+ // equality delete
+ table.newRowDelta().addDeletes(FILE_C2_DELETES).commit();
+ Snapshot snap3 = table.currentSnapshot();
+
+ // mix of position and equality deletes
+
table.newRowDelta().addDeletes(FILE_A_DELETES).addDeletes(FILE_A2_DELETES).commit();
+ Snapshot snap4 = table.currentSnapshot();
+
+ IncrementalChangelogScan scan =
+
newScan().fromSnapshotExclusive(snap1.snapshotId()).toSnapshot(snap4.snapshotId());
+
+ List<ChangelogScanTask> tasks = plan(scan);
+
+ assertThat(tasks).as("Must have 4 tasks").hasSize(4);
+
+ DeletedRowsScanTask t1 = (DeletedRowsScanTask) tasks.get(0);
+ assertThat(t1.changeOrdinal()).as("Ordinal must match").isEqualTo(0);
+ assertThat(t1.commitSnapshotId()).as("Snapshot must
match").isEqualTo(snap2.snapshotId());
+ assertThat(t1.file().path()).as("Data file must
match").isEqualTo(FILE_B.path());
+ assertThat(t1.addedDeletes().get(0).path())
+ .as("Added delete file must match")
+ .isEqualTo(FILE_B_DELETES.path());
+ assertThat(t1.existingDeletes()).as("Must be no existing
deletes").isEmpty();
+
+ DeletedRowsScanTask t2 = (DeletedRowsScanTask) tasks.get(1);
+ assertThat(t2.changeOrdinal()).as("Ordinal must match").isEqualTo(1);
+ assertThat(t2.commitSnapshotId()).as("Snapshot must
match").isEqualTo(snap3.snapshotId());
+ assertThat(t2.file().path()).as("Data file must
match").isEqualTo(FILE_C.path());
+ assertThat(t2.addedDeletes().get(0).path())
+ .as("Added delete file must match")
+ .isEqualTo(FILE_C2_DELETES.path());
+ assertThat(t2.existingDeletes()).as("Must be no existing
deletes").isEmpty();
+
+ DeletedRowsScanTask t3 = (DeletedRowsScanTask) tasks.get(2);
+ assertThat(t3.changeOrdinal()).as("Ordinal must match").isEqualTo(2);
+ assertThat(t3.commitSnapshotId()).as("Snapshot must
match").isEqualTo(snap4.snapshotId());
+ assertThat(t3.file().path()).as("Data file must
match").isEqualTo(FILE_A2.path());
+ assertThat(t3.addedDeletes().size()).as("Number of added delete files must
match").isEqualTo(2);
+ assertThat(t3.addedDeletes().get(0).path())
+ .as("Added delete file must match")
+ .isEqualTo(FILE_A2_DELETES.path());
+ assertThat(t3.addedDeletes().get(1).path())
+ .as("Added delete file must match")
+ .isEqualTo(FILE_A_DELETES.path());
Review Comment:
if I understand this correctly, the test result seem to be consistent here
as when build DeleteFileIndex, we always concat equality delete (A2_DELETES)
before position delete (A_DELETES) for given data file entry in
https://github.com/apache/iceberg/blob/9dbfbbb203c007592b8ba5f4816925043c08923a/core/src/main/java/org/apache/iceberg/DeleteFileIndex.java#L139
and array ordering seem to be maintained through out the code.
IMO, this is not necessarily something we need to assert the ordering on
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
