openinx commented on a change in pull request #2294:
URL: https://github.com/apache/iceberg/pull/2294#discussion_r596637870
##########
File path: core/src/test/java/org/apache/iceberg/TestRewriteFiles.java
##########
@@ -228,6 +383,163 @@ public void testRecovery() {
Assert.assertEquals("Only 3 manifests should exist", 3,
listManifestFiles().size());
}
+ @Test
+ public void testRecoverWhenRewriteBothDataAndDeleteFiles() {
+ Assume.assumeTrue("Rewriting delete files is only supported in iceberg
format v2. ", formatVersion > 1);
+
+ table.newRowDelta()
+ .addRows(FILE_A)
+ .addRows(FILE_B)
+ .addRows(FILE_C)
+ .addDeletes(FILE_A_DELETES)
+ .addDeletes(FILE_B_DELETES)
+ .commit();
+
+ long baseSnapshotId = readMetadata().currentSnapshot().snapshotId();
+ table.ops().failCommits(3);
+
+ RewriteFiles rewrite = table.newRewrite()
+ .rewriteFiles(ImmutableSet.of(FILE_A), ImmutableSet.of(FILE_A_DELETES,
FILE_B_DELETES),
+ ImmutableSet.of(FILE_D), ImmutableSet.of());
+ Snapshot pending = rewrite.apply();
+
+ Assert.assertEquals("Should produce 3 manifests", 3,
pending.allManifests().size());
+ ManifestFile manifest1 = pending.allManifests().get(0);
+ ManifestFile manifest2 = pending.allManifests().get(1);
+ ManifestFile manifest3 = pending.allManifests().get(2);
+
+ validateManifestEntries(manifest1,
+ ids(pending.snapshotId()),
+ files(FILE_D),
+ statuses(ADDED));
+
+ validateManifestEntries(manifest2,
+ ids(pending.snapshotId(), baseSnapshotId, baseSnapshotId),
+ files(FILE_A, FILE_B, FILE_C),
+ statuses(DELETED, EXISTING, EXISTING));
+
+ validateDeleteManifest(manifest3,
+ seqs(2, 2),
+ ids(pending.snapshotId(), pending.snapshotId()),
+ files(FILE_A_DELETES, FILE_B_DELETES),
+ statuses(DELETED, DELETED));
+
+ rewrite.commit();
+
+ Assert.assertTrue("Should reuse new manifest", new
File(manifest1.path()).exists());
+ Assert.assertTrue("Should reuse new manifest", new
File(manifest2.path()).exists());
+ Assert.assertTrue("Should reuse new manifest", new
File(manifest3.path()).exists());
+
+ TableMetadata metadata = readMetadata();
+ List<ManifestFile> committedManifests = Lists.newArrayList(manifest1,
manifest2, manifest3);
+ Assert.assertTrue("Should committed the manifests",
+
metadata.currentSnapshot().allManifests().containsAll(committedManifests));
+
+ // As commit success all the manifests added with rewrite should be
available.
+ Assert.assertEquals("Only 5 manifest should exist", 5,
listManifestFiles().size());
+ }
+
+ @Test
+ public void testReplaceEqualityDeletesWithPositionDeletes() {
+ Assume.assumeTrue("Rewriting delete files is only supported in iceberg
format v2. ", formatVersion > 1);
+
+ table.newRowDelta()
+ .addRows(FILE_A2)
+ .addDeletes(FILE_A2_DELETES)
+ .commit();
+
+ TableMetadata metadata = readMetadata();
+ long baseSnapshotId = metadata.currentSnapshot().snapshotId();
+
+ // Apply and commit the rewrite transaction.
+ RewriteFiles rewrite = table.newRewrite().rewriteFiles(
+ ImmutableSet.of(), ImmutableSet.of(FILE_A2_DELETES),
+ ImmutableSet.of(), ImmutableSet.of(FILE_B_DELETES)
+ );
+ Snapshot pending = rewrite.apply();
+
+ Assert.assertEquals("Should produce 3 manifests", 3,
pending.allManifests().size());
+ ManifestFile manifest1 = pending.allManifests().get(0);
+ ManifestFile manifest2 = pending.allManifests().get(1);
+ ManifestFile manifest3 = pending.allManifests().get(2);
+
+ validateManifestEntries(manifest1,
+ ids(baseSnapshotId),
Review comment:
I think we'd better to add the seq number verification after we have
reached agreement on this issue: https://github.com/apache/iceberg/issues/2308.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
