mxm commented on code in PR #17630:
URL: https://github.com/apache/iceberg/pull/17630#discussion_r3843362988
##########
flink/v2.1/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestConvertEqualityDeletes.java:
##########
@@ -1348,15 +1348,30 @@ void testDeleteResolvedBeforeFailureIsRetained() throws
Exception {
insert(table, 1, "a");
insert(table, 2, "b");
- // Two eq deletes in one commit. id=1's delete file stays readable so it
resolves, while id=2's
- // is removed so the cycle aborts only after id=1 has been resolved out of
the index.
+ // Two eq deletes with their re-inserts in one commit, the usual upsert
shape. id=1's delete
+ // file
+ // stays readable so it resolves, while id=2's is removed so the cycle
aborts only after id=1
+ // has
+ // been resolved out of the index.
Review Comment:
Indention is off here.
##########
flink/v2.1/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestConvertEqualityDeletes.java:
##########
@@ -1395,9 +1410,10 @@ void testDeleteResolvedBeforeFailureIsRetained() throws
Exception {
assertThat(result2.success()).isTrue();
table.refresh();
- // The retried cycle converted both eq deletes, so both rows stay
deleted.
+ // The retried cycle converted both eq deletes, so only the re-inserts
remain visible.
assertNoEqualityDeletesOnMain(table, 0);
- assertRecords(table, ImmutableList.of());
+ assertRecords(table, ImmutableList.of(createRecord(1, "a"),
createRecord(2, "b")));
+ assertThat(visibleRowCount(table)).isEqualTo(2);
Review Comment:
Isn't this check redundant? We already check in the line prior to this line
that there are only two rows.
##########
flink/v2.1/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestConvertEqualityDeletes.java:
##########
@@ -1488,6 +1504,22 @@ private void appendConvertTask(String stagingBranch) {
.sinkTo(infra.sink());
}
+ private static long visibleRowCount(Table table) throws IOException {
+ table.refresh();
+ long count = 0;
+ try (CloseableIterable<Record> iterable =
+ IcebergGenerics.read(table)
+ .useSnapshot(table.currentSnapshot().snapshotId())
+ .project(SimpleDataUtil.SCHEMA)
+ .build()) {
+ for (Record ignored : iterable) {
+ count++;
+ }
+ }
+
+ return count;
+ }
Review Comment:
I think this method can be removed.
##########
flink/v2.1/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestConvertEqualityDeletes.java:
##########
@@ -1348,15 +1348,30 @@ void testDeleteResolvedBeforeFailureIsRetained() throws
Exception {
insert(table, 1, "a");
insert(table, 2, "b");
- // Two eq deletes in one commit. id=1's delete file stays readable so it
resolves, while id=2's
- // is removed so the cycle aborts only after id=1 has been resolved out of
the index.
+ // Two eq deletes with their re-inserts in one commit, the usual upsert
shape. id=1's delete
+ // file
+ // stays readable so it resolves, while id=2's is removed so the cycle
aborts only after id=1
+ // has
+ // been resolved out of the index.
+ DataFile reinsertA = writeDataFile(table, createRecord(1, "a"));
+ DataFile reinsertB = writeDataFile(table, createRecord(2, "b"));
DeleteFile readableDelete = writeEqualityDelete(table, 1, "a");
DeleteFile missingDelete = writeEqualityDelete(table, 2, "b");
-
table.newRowDelta().addDeletes(readableDelete).addDeletes(missingDelete).commit();
+ table
+ .newRowDelta()
+ .addRows(reinsertA)
+ .addRows(reinsertB)
+ .addDeletes(readableDelete)
+ .addDeletes(missingDelete)
+ .commit();
table.refresh();
- // Both rows are hidden by the eq deletes before the conversion runs.
- assertRecords(table, ImmutableList.of());
+ // The eq deletes carry this commit's sequence number, so they hide the
original rows but not
+ // the
+ // re-inserts. The row count matters as well as the values: a delete that
goes missing leaves a
+ // second copy of an identical row, which a set comparison alone would not
catch.
+ assertRecords(table, ImmutableList.of(createRecord(1, "a"),
createRecord(2, "b")));
+ assertThat(visibleRowCount(table)).isEqualTo(2);
Review Comment:
Same here.
--
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]
