wypoon commented on code in PR #10935:
URL: https://github.com/apache/iceberg/pull/10935#discussion_r1765955792
##########
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());
+ assertThat(t3.existingDeletes()).as("Must be no existing
deletes").isEmpty();
+
+ DeletedRowsScanTask t4 = (DeletedRowsScanTask) tasks.get(3);
+ assertThat(t4.changeOrdinal()).as("Ordinal must match").isEqualTo(2);
+ assertThat(t4.commitSnapshotId()).as("Snapshot must
match").isEqualTo(snap4.snapshotId());
+ assertThat(t4.file().path()).as("Data file must
match").isEqualTo(FILE_A.path());
+ assertThat(t4.addedDeletes().size()).as("Number of added delete files must
match").isEqualTo(2);
+ assertThat(t4.addedDeletes().get(0).path())
+ .as("Added delete file must match")
+ .isEqualTo(FILE_A2_DELETES.path());
+ assertThat(t4.addedDeletes().get(1).path())
+ .as("Added delete file must match")
+ .isEqualTo(FILE_A_DELETES.path());
+ assertThat(t4.existingDeletes()).as("Must be no existing
deletes").isEmpty();
Review Comment:
Thanks for thinking about this and the idea. I learned something about
AssertJ from you!
As I see it, however, the example you show does not actually result in
reduction in code, just that it is organized differently. Also, as far as using
the `RecursiveComparisonConfiguration` goes, that part really does not reduce
code at all. Conceptually, we want to check what the
deletes/addedDeletes/existingDeletes are, and it is enough to check that the
file path matches with the path of the `DeleteFile`s we expect. In other words,
```
assertThat(t4.addedDeletes().get(0).path())
.isEqualTo(FILE_A2_DELETES.path());
```
is just as good as
```
assertThat(t4.addedDeletes().get(0))
.usingRecursiveComparison(FILE_COMPARISON_CONFIG)
.isEqualTo(FILE_A2_DELETES);
```
for our purposes, and is slightly shorter.
--
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
