wombatu-kun commented on code in PR #16669:
URL: https://github.com/apache/iceberg/pull/16669#discussion_r3345449791
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -281,4 +281,51 @@ public void
testRewritingMultiplePositionDeleteEntriesWithinManifestFile() throw
assertThat(deleteFileRewriteResult.toRewrite()).hasSize(2);
}
+
+ @TestTemplate
+ public void testDeletedPositionDeleteEntriesAreNotRewritten() throws
IOException {
+ assumeThat(formatVersion)
+ .as("Delete files only work for format version 2")
+ .isGreaterThanOrEqualTo(2);
+
+ String sourcePrefix = "/path/to/";
+ String stagingDir = "/staging/";
+ String targetPrefix = "/path/new/";
+ long snapshotId = 1000L;
+
+ DeleteFile oldPositionDeletes =
+ FileMetadata.deleteFileBuilder(table.spec())
+ .ofPositionDeletes()
+ .withPath("/path/to/old-data-deletes.parquet")
+ .withFileSizeInBytes(10)
+ .withPartitionPath("data_bucket=0")
+ .withRecordCount(1)
+ .build();
+
+ ManifestFile manifest =
+ writeManifest(
+ snapshotId,
+ manifestEntry(ManifestEntry.Status.DELETED, snapshotId,
oldPositionDeletes),
+ manifestEntry(ManifestEntry.Status.ADDED, snapshotId,
FILE_A_DELETES));
+
+ RewriteTablePathUtil.RewriteResult<DeleteFile> deleteFileRewriteResult =
+ RewriteTablePathUtil.rewriteDeleteManifest(
+ manifest,
+ Set.of(snapshotId),
+ Files.localOutput(
+ FileFormat.AVRO.addExtension(
+ temp.resolve("junit" +
System.nanoTime()).toFile().toString())),
+ table.io(),
+ formatVersion,
+ table.specs(),
+ sourcePrefix,
+ targetPrefix,
+ stagingDir);
+
+ assertThat(deleteFileRewriteResult.toRewrite())
+ .hasSize(1)
+ .extracting(DeleteFile::location)
+ .containsExactly(FILE_A_DELETES.location());
+ assertThat(deleteFileRewriteResult.copyPlan()).hasSize(1);
Review Comment:
This assertion doesn't exercise the fix: `copyPlan()` was already gated on
`entry.isLive()` and is 1 both before and after the change, so it passes
regardless of the regression. Please remove it - the `toRewrite()` assertion
above is what pins the behavior.
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -464,8 +464,8 @@ private static RewriteResult<DeleteFile>
writeDeleteFileEntry(
Pair.of(
stagingPath(file.location(), sourcePrefix,
stagingLocation),
posDeleteFile.location()));
+ result.toRewrite().add(file.copy());
Review Comment:
The `// keep the following entries in metadata but exclude them from
copyPlan` comment just above this block now also covers `toRewrite`, since this
line sits inside the same `isLive()` gate. Could you update it to read `...
exclude them from copyPlan and toRewrite` so it stays accurate?
--
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]
