From d1234eedf691a88a01337b6910c9babae28a3c97 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 27 Feb 2026 12:08:29 -0500 Subject: [PATCH 01/17] Move position delete file rewriting into the manifest-writing Spark task, eliminating the separate `rewritePositionDeletes()` Spark job. Each manifest-writing task now also rewrites the delete files that manifest references, measures the actual size via `getLength()`, and records it in the manifest entry. --- .palantir/revapi.yml | 7 ++ .../apache/iceberg/RewriteTablePathUtil.java | 48 +++++++++---- .../actions/RewriteTablePathSparkAction.java | 71 ++++--------------- .../actions/TestRewriteTablePathsAction.java | 56 ++++++++++++--- 4 files changed, 105 insertions(+), 77 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index cd4afe6fdc8d..d40a2db443d3 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -1422,6 +1422,13 @@ acceptedBreaks: \ org.apache.iceberg.PartitionSpec>, java.lang.String, java.lang.String, java.lang.String)\ \ throws java.io.IOException" justification: "Removing deprecated code for 1.11.0" + - code: "java.method.numberOfParametersChanged" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map, java.lang.String,\ + \ java.lang.String, java.lang.String) throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest writing to fix file_size_in_bytes" - code: "java.method.removed" old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" justification: "Removing deprecated code for 1.11.0" diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index 435f79129204..0e79fd1a5c2e 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -366,6 +366,9 @@ public static RewriteResult rewriteDataManifest( /** * Rewrite a delete manifest, replacing path references. * + *

Position delete files are rewritten inline so that the manifest records the actual file size + * after path rewriting. + * * @param manifestFile source delete manifest to rewrite * @param snapshotIds snapshot ids for filtering returned delete manifest entries * @param outputFile output file to rewrite manifest file to @@ -374,8 +377,8 @@ public static RewriteResult rewriteDataManifest( * @param specsById map of partition specs by id * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix that will replace it - * @param stagingLocation staging location for rewritten files (referred delete file will be - * rewritten here) + * @param stagingLocation staging location for rewritten position delete files + * @param posDeleteReaderWriter reader/writer for position delete files * @return a copy plan of content files in the manifest that was rewritten */ public static RewriteResult rewriteDeleteManifest( @@ -387,7 +390,8 @@ public static RewriteResult rewriteDeleteManifest( Map specsById, String sourcePrefix, String targetPrefix, - String stagingLocation) + String stagingLocation, + PositionDeleteReaderWriter posDeleteReaderWriter) throws IOException { PartitionSpec spec = specsById.get(manifestFile.partitionSpecId()); try (ManifestWriter writer = @@ -405,7 +409,9 @@ public static RewriteResult rewriteDeleteManifest( sourcePrefix, targetPrefix, stagingLocation, - writer)) + writer, + io, + posDeleteReaderWriter)) .reduce(new RewriteResult<>(), RewriteResult::append); } } @@ -445,25 +451,36 @@ private static RewriteResult writeDeleteFileEntry( String sourcePrefix, String targetPrefix, String stagingLocation, - ManifestWriter writer) { + ManifestWriter writer, + FileIO io, + PositionDeleteReaderWriter posDeleteReaderWriter) { DeleteFile file = entry.file(); RewriteResult result = new RewriteResult<>(); switch (file.content()) { case POSITION_DELETES: - DeleteFile posDeleteFile = newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix); + // Rewrite inline so the manifest records the actual file size, which changes because + // embedded data file paths are rewritten. The staging path is deterministic, so + // duplicates across manifests simply overwrite with identical content. + String staging = stagingPath(file.location(), sourcePrefix, stagingLocation); + OutputFile outputFile = io.newOutputFile(staging); + try { + rewritePositionDeleteFile( + file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); + } catch (IOException e) { + throw new UncheckedIOException( + "Failed to rewrite position delete file " + file.location(), e); + } + long actualSize = io.newInputFile(staging).getLength(); + DeleteFile posDeleteFile = + newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, actualSize); appendEntryWithFile(entry, writer, posDeleteFile); // keep the following entries in metadata but exclude them from copyPlan // 1) deleted position delete files // 2) entries not changed by snapshotIds if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) { - result - .copyPlan() - .add( - Pair.of( - stagingPath(file.location(), sourcePrefix, stagingLocation), - posDeleteFile.location())); + result.copyPlan().add(Pair.of(staging, posDeleteFile.location())); } result.toRewrite().add(file.copy()); return result; @@ -523,7 +540,11 @@ private static DeleteFile newEqualityDeleteEntry( } private static DeleteFile newPositionDeleteEntry( - DeleteFile file, PartitionSpec spec, String sourcePrefix, String targetPrefix) { + DeleteFile file, + PartitionSpec spec, + String sourcePrefix, + String targetPrefix, + long fileSizeInBytes) { String path = file.location(); Preconditions.checkArgument( path.startsWith(sourcePrefix), @@ -535,6 +556,7 @@ private static DeleteFile newPositionDeleteEntry( FileMetadata.deleteFileBuilder(spec) .copy(file) .withPath(newPath(path, sourcePrefix, targetPrefix)) + .withFileSizeInBytes(fileSizeInBytes) .withMetrics(ContentFileUtil.replacePathBounds(file, sourcePrefix, targetPrefix)); // Update referencedDataFile for DV files diff --git a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index aedb25e4a4a6..cd420ae40b12 100644 --- a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -72,10 +72,8 @@ import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.JobGroupInfo; import org.apache.iceberg.spark.source.SerializableTableWithSize; -import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; -import org.apache.spark.api.java.function.ForeachFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -313,18 +311,14 @@ private Result rebuildMetadata() { RewriteContentFileResult rewriteManifestResult = rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - // rebuild position delete files - Set deleteFiles = - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)); - rewritePositionDeletes(deleteFiles); + int rewrittenDeleteFilesCount = + (int) + rewriteManifestResult.toRewrite().stream().filter(e -> e instanceof DeleteFile).count(); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(deleteFiles.size()) + .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -572,6 +566,7 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); return manifestDS .repartition(toRewrite.size()) .map( @@ -581,7 +576,8 @@ private RewriteContentFileResult rewriteManifests( stagingDir, tableMetadata.formatVersion(), sourcePrefix, - targetPrefix), + targetPrefix, + posDeleteReaderWriter), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -594,7 +590,8 @@ private static MapFunction toManifests( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -619,7 +616,8 @@ private static MapFunction toManifests( stagingLocation, format, sourcePrefix, - targetPrefix)); + targetPrefix, + posDeleteReaderWriter)); break; default: throw new UnsupportedOperationException( @@ -665,7 +663,8 @@ private static RewriteResult writeDeleteManifest( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -682,29 +681,13 @@ private static RewriteResult writeDeleteManifest( specsById, sourcePrefix, targetPrefix, - stagingLocation); + stagingLocation, + posDeleteReaderWriter); } catch (IOException e) { throw new RuntimeIOException(e); } } - private void rewritePositionDeletes(Set toRewrite) { - if (toRewrite.isEmpty()) { - return; - } - - Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDs = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); - - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); - deleteFileDs - .repartition(toRewrite.size()) - .foreach( - rewritePositionDelete( - tableBroadcast(), sourcePrefix, targetPrefix, stagingDir, posDeleteReaderWriter)); - } - private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( @@ -724,30 +707,6 @@ public PositionDeleteWriter writer( } } - private ForeachFunction rewritePositionDelete( - Broadcast tableArg, - String sourcePrefixArg, - String targetPrefixArg, - String stagingLocationArg, - PositionDeleteReaderWriter posDeleteReaderWriter) { - return deleteFile -> { - FileIO io = tableArg.getValue().io(); - String newPath = - RewriteTablePathUtil.stagingPath( - deleteFile.location(), sourcePrefixArg, stagingLocationArg); - OutputFile outputFile = io.newOutputFile(newPath); - PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); - RewriteTablePathUtil.rewritePositionDeleteFile( - deleteFile, - outputFile, - io, - spec, - sourcePrefixArg, - targetPrefixArg, - posDeleteReaderWriter); - }; - } - private static CloseableIterable positionDeletesReader( InputFile inputFile, FileFormat format, PartitionSpec spec) { return FormatModelRegistry.readBuilder(format, Record.class, inputFile) diff --git a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index dae721b1d73d..f18525ad7823 100644 --- a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -42,6 +42,9 @@ import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; import org.apache.iceberg.HasTableOperations; +import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestFiles; +import org.apache.iceberg.ManifestReader; import org.apache.iceberg.Parameter; import org.apache.iceberg.ParameterizedTestExtension; import org.apache.iceberg.Parameters; @@ -629,12 +632,7 @@ public void testPositionDeletesDeduplication() throws Exception { // in a new manifest, which will cause duplicate DeleteFile objects when processing tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); - // This should NOT throw AlreadyExistsException - the fix uses DeleteFileSet to deduplicate - // Without the fix (using Collectors.toSet()), this would fail because: - // 1. Both manifests contain entries for the same delete file - // 2. Processing returns two different DeleteFile objects for the same file - // 3. HashSet doesn't deduplicate them (DeleteFile doesn't override equals()) - // 4. rewritePositionDeletes tries to write the same file twice -> AlreadyExistsException + // This should NOT throw AlreadyExistsException RewriteTablePath.Result result = actions() .rewriteTablePath(tableWithPosDeletes) @@ -642,13 +640,55 @@ public void testPositionDeletesDeduplication() throws Exception { .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) .execute(); - // Verify the rewrite completed successfully - should have rewritten exactly 1 delete file - // (the duplicate should be deduplicated by DeleteFileSet) assertThat(result.rewrittenDeleteFilePathsCount()) .as("Should have rewritten exactly 1 delete file after deduplication") .isEqualTo(1); } + // Regression test: rewriting delete file paths changes the file size (since the + // embedded data file paths may differ in length), but file_size_in_bytes in the rewritten + // manifest was not updated. Readers that use file_size_in_bytes to elide a stat() call may + // fail. + @TestTemplate + public void testDeleteFileSizeInBytesAfterRewrite() throws Exception { + List> deletes = + Lists.newArrayList( + Pair.of( + table.currentSnapshot().addedDataFiles(table.io()).iterator().next().location(), + 0L)); + + File file = new File(removePrefix(table.location() + "/data/deeply/nested/deletes.parquet")); + DeleteFile positionDeletes = + FileHelpers.writeDeleteFile( + table, table.io().newOutputFile(file.toURI().toString()), deletes, formatVersion) + .first(); + table.newRowDelta().addDeletes(positionDeletes).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(table) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(table.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + } + } + } + } + @TestTemplate public void testEqualityDeletes() throws Exception { Table sourceTable = createTableWithSnapshots(newTableLocation(), 1); From 285274512f31f60ef63fac9b4e1c27f5dad27a5d Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 27 Feb 2026 12:26:05 -0500 Subject: [PATCH 02/17] Fix API check? --- .palantir/revapi.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index d40a2db443d3..9b3216b105a1 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -1429,6 +1429,13 @@ acceptedBreaks: \ int, java.util.Map, java.lang.String,\ \ java.lang.String, java.lang.String) throws java.io.IOException" justification: "Position delete files are now rewritten inline during manifest writing to fix file_size_in_bytes" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map, java.lang.String,\ + \ java.lang.String, java.lang.String) throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest writing to fix file_size_in_bytes" - code: "java.method.removed" old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" justification: "Removing deprecated code for 1.11.0" From 4b0bddd655e35f5e8a9c73c2529f5df66c42d78e Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 27 Feb 2026 12:39:42 -0500 Subject: [PATCH 03/17] Fix API check? --- .palantir/revapi.yml | 137 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 9b3216b105a1..84fd4bef8a10 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -404,6 +404,143 @@ acceptedBreaks: old: "method org.apache.iceberg.orc.ORC.WriteBuilder org.apache.iceberg.orc.ORC.WriteBuilder::config(java.lang.String,\ \ java.lang.String)" justification: "Removing deprecations for 1.2.0" + "1.10.0": + org.apache.iceberg:iceberg-api: + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.encryption.EncryptingFileIO" + new: "class org.apache.iceberg.encryption.EncryptingFileIO" + justification: "New method for Manifest List reading" + org.apache.iceberg:iceberg-core: + - code: "java.class.noLongerInheritsFromClass" + old: "class org.apache.iceberg.rest.auth.OAuth2Manager" + new: "class org.apache.iceberg.rest.auth.OAuth2Manager" + justification: "Removing deprecations for 1.11.0" + - code: "java.class.nowImplementsInterface" + old: "class org.apache.iceberg.rest.auth.OAuth2Manager" + new: "class org.apache.iceberg.rest.auth.OAuth2Manager" + justification: "Removing deprecations for 1.11.0" + - code: "java.class.removed" + old: "class org.apache.iceberg.PartitionStatsUtil" + justification: "Removing deprecated code for 1.11.0" + - code: "java.class.removed" + old: "class org.apache.iceberg.rest.auth.RefreshingAuthManager" + justification: "Removing deprecations for 1.11.0" + - code: "java.element.noLongerDeprecated" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" + - code: "java.field.constantValueChanged" + old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" + new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" + justification: "Plan API is table scoped and path constant value should include\ + \ namespace. No actual breakage because it never worked before with incorrect\ + \ value." + - code: "java.field.constantValueChanged" + old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" + new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" + justification: "Plan API is table scoped and path constant value should include\ + \ namespace. No actual breakage because it never worked before with incorrect\ + \ value." + - code: "java.field.constantValueChanged" + old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" + new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" + justification: "Plan API is table scoped and path constant value should include\ + \ namespace. No actual breakage because it never worked before with incorrect\ + \ value." + - code: "java.method.numberOfParametersChanged" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile,org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" + - code: "java.method.numberOfParametersChanged" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" + - code: "java.method.removed" + old: "method java.lang.String org.apache.iceberg.RewriteTablePathUtil::stagingPath(java.lang.String,\ + \ java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDataManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String) throws\ + \ java.io.IOException" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.TableMetadata org.apache.iceberg.TableMetadataParser::read(org.apache.iceberg.io.FileIO,\ + \ org.apache.iceberg.io.InputFile)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.encryption.EncryptionManager org.apache.iceberg.encryption.EncryptionUtil::createEncryptionManager(java.util.Map, org.apache.iceberg.encryption.KeyManagementClient)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String,\ + \ java.lang.String, java.lang.String, java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String,\ + \ java.lang.String, java.lang.String, java.lang.String, java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String,\ + \ java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.visibilityReduced" + old: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" + new: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" + justification: "Changing deprecated code" + - code: "java.method.visibilityReduced" + old: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" + new: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" + justification: "Changing deprecated code" + - code: "java.method.visibilityReduced" + old: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile,\ + \ org.apache.iceberg.Snapshot)" + new: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile,\ + \ org.apache.iceberg.Snapshot)" + justification: "Changing deprecated code" + org.apache.iceberg:iceberg-data: + - code: "java.class.removed" + old: "class org.apache.iceberg.data.PartitionStatsHandler" + justification: "Removing deprecated code for 1.11.0" "1.2.0": org.apache.iceberg:iceberg-api: - code: "java.field.constantValueChanged" From ceb65bc12c24f10c4604f1fa49641f002c4bdd07 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Tue, 10 Mar 2026 17:29:23 -0400 Subject: [PATCH 04/17] Address PR feedback 2 and 4. --- .../java/org/apache/iceberg/RewriteTablePathUtil.java | 8 ++++---- .../spark/actions/RewriteTablePathSparkAction.java | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index 0e79fd1a5c2e..bf9034b55e9b 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -463,8 +463,8 @@ private static RewriteResult writeDeleteFileEntry( // Rewrite inline so the manifest records the actual file size, which changes because // embedded data file paths are rewritten. The staging path is deterministic, so // duplicates across manifests simply overwrite with identical content. - String staging = stagingPath(file.location(), sourcePrefix, stagingLocation); - OutputFile outputFile = io.newOutputFile(staging); + String stagingPath = stagingPath(file.location(), sourcePrefix, stagingLocation); + OutputFile outputFile = io.newOutputFile(stagingPath); try { rewritePositionDeleteFile( file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); @@ -472,7 +472,7 @@ private static RewriteResult writeDeleteFileEntry( throw new UncheckedIOException( "Failed to rewrite position delete file " + file.location(), e); } - long actualSize = io.newInputFile(staging).getLength(); + long actualSize = io.newInputFile(stagingPath).getLength(); DeleteFile posDeleteFile = newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, actualSize); appendEntryWithFile(entry, writer, posDeleteFile); @@ -480,7 +480,7 @@ private static RewriteResult writeDeleteFileEntry( // 1) deleted position delete files // 2) entries not changed by snapshotIds if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) { - result.copyPlan().add(Pair.of(staging, posDeleteFile.location())); + result.copyPlan().add(Pair.of(stagingPath, posDeleteFile.location())); } result.toRewrite().add(file.copy()); return result; diff --git a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index cd420ae40b12..a6bac705b86a 100644 --- a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -313,7 +313,10 @@ private Result rebuildMetadata() { int rewrittenDeleteFilesCount = (int) - rewriteManifestResult.toRewrite().stream().filter(e -> e instanceof DeleteFile).count(); + rewriteManifestResult.toRewrite().stream() + .filter(e -> e instanceof DeleteFile) + .distinct() + .count(); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() From f98a69298437de388d37991ff29224fa287ddd30 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Tue, 10 Mar 2026 17:38:20 -0400 Subject: [PATCH 05/17] Try to reduce revapi diff. --- .palantir/revapi.yml | 137 ------------------------------------------- 1 file changed, 137 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 84fd4bef8a10..9b3216b105a1 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -404,143 +404,6 @@ acceptedBreaks: old: "method org.apache.iceberg.orc.ORC.WriteBuilder org.apache.iceberg.orc.ORC.WriteBuilder::config(java.lang.String,\ \ java.lang.String)" justification: "Removing deprecations for 1.2.0" - "1.10.0": - org.apache.iceberg:iceberg-api: - - code: "java.class.defaultSerializationChanged" - old: "class org.apache.iceberg.encryption.EncryptingFileIO" - new: "class org.apache.iceberg.encryption.EncryptingFileIO" - justification: "New method for Manifest List reading" - org.apache.iceberg:iceberg-core: - - code: "java.class.noLongerInheritsFromClass" - old: "class org.apache.iceberg.rest.auth.OAuth2Manager" - new: "class org.apache.iceberg.rest.auth.OAuth2Manager" - justification: "Removing deprecations for 1.11.0" - - code: "java.class.nowImplementsInterface" - old: "class org.apache.iceberg.rest.auth.OAuth2Manager" - new: "class org.apache.iceberg.rest.auth.OAuth2Manager" - justification: "Removing deprecations for 1.11.0" - - code: "java.class.removed" - old: "class org.apache.iceberg.PartitionStatsUtil" - justification: "Removing deprecated code for 1.11.0" - - code: "java.class.removed" - old: "class org.apache.iceberg.rest.auth.RefreshingAuthManager" - justification: "Removing deprecations for 1.11.0" - - code: "java.element.noLongerDeprecated" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ - \ throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" - - code: "java.field.constantValueChanged" - old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" - new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" - justification: "Plan API is table scoped and path constant value should include\ - \ namespace. No actual breakage because it never worked before with incorrect\ - \ value." - - code: "java.field.constantValueChanged" - old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" - new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" - justification: "Plan API is table scoped and path constant value should include\ - \ namespace. No actual breakage because it never worked before with incorrect\ - \ value." - - code: "java.field.constantValueChanged" - old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" - new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" - justification: "Plan API is table scoped and path constant value should include\ - \ namespace. No actual breakage because it never worked before with incorrect\ - \ value." - - code: "java.method.numberOfParametersChanged" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ java.util.Set, org.apache.iceberg.io.OutputFile,org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map,\ - \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" - - code: "java.method.numberOfParametersChanged" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ - \ throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" - - code: "java.method.removed" - old: "method java.lang.String org.apache.iceberg.RewriteTablePathUtil::stagingPath(java.lang.String,\ - \ java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDataManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String) throws\ - \ java.io.IOException" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map,\ - \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" - - code: "java.method.removed" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ - \ throws java.io.IOException" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.TableMetadata org.apache.iceberg.TableMetadataParser::read(org.apache.iceberg.io.FileIO,\ - \ org.apache.iceberg.io.InputFile)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.encryption.EncryptionManager org.apache.iceberg.encryption.EncryptionUtil::createEncryptionManager(java.util.Map, org.apache.iceberg.encryption.KeyManagementClient)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String,\ - \ java.lang.String, java.lang.String, java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String,\ - \ java.lang.String, java.lang.String, java.lang.String, java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String,\ - \ java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.visibilityReduced" - old: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" - new: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" - justification: "Changing deprecated code" - - code: "java.method.visibilityReduced" - old: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" - new: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" - justification: "Changing deprecated code" - - code: "java.method.visibilityReduced" - old: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile,\ - \ org.apache.iceberg.Snapshot)" - new: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile,\ - \ org.apache.iceberg.Snapshot)" - justification: "Changing deprecated code" - org.apache.iceberg:iceberg-data: - - code: "java.class.removed" - old: "class org.apache.iceberg.data.PartitionStatsHandler" - justification: "Removing deprecated code for 1.11.0" "1.2.0": org.apache.iceberg:iceberg-api: - code: "java.field.constantValueChanged" From d0f994430ae3b89d8b2bb3489e304acc71e94f60 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Tue, 10 Mar 2026 18:51:52 -0400 Subject: [PATCH 06/17] Try to reduce revapi diff. --- .palantir/revapi.yml | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 9b3216b105a1..95f274c6d9e8 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -1370,14 +1370,6 @@ acceptedBreaks: new: "class org.apache.iceberg.encryption.EncryptingFileIO" justification: "New method for Manifest List reading" org.apache.iceberg:iceberg-core: - - code: "java.class.defaultSerializationChanged" - old: "class org.apache.iceberg.avro.SupportsIndexProjection" - new: "class org.apache.iceberg.avro.SupportsIndexProjection" - justification: "Serialization across versions is not guaranteed" - - code: "java.class.defaultSerializationChanged" - old: "class org.apache.iceberg.hadoop.SerializableConfiguration" - new: "class org.apache.iceberg.hadoop.SerializableConfiguration" - justification: "Serialization across versions is not guaranteed" - code: "java.class.noLongerInheritsFromClass" old: "class org.apache.iceberg.rest.auth.OAuth2Manager" new: "class org.apache.iceberg.rest.auth.OAuth2Manager" @@ -1416,26 +1408,41 @@ acceptedBreaks: \ java.io.IOException" justification: "Removing deprecated code for 1.11.0" - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" + - code: "java.method.numberOfParametersChanged" old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ \ throws java.io.IOException" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.numberOfParametersChanged" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + new: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map, java.lang.String,\ - \ java.lang.String, java.lang.String) throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest writing to fix file_size_in_bytes" - - code: "java.method.removed" + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ + \ throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" + - code: "java.element.noLongerDeprecated" old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + new: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map, java.lang.String,\ - \ java.lang.String, java.lang.String) throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest writing to fix file_size_in_bytes" + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ + \ throws java.io.IOException" + justification: "Position delete files are now rewritten inline during manifest\ + \ writing to fix file_size_in_bytes" - code: "java.method.removed" old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" justification: "Removing deprecated code for 1.11.0" From 517c018cd140af3ece5016515b46bbe9ee1ded0f Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Wed, 15 Apr 2026 13:54:26 -0400 Subject: [PATCH 07/17] rebased on upstream main --- .palantir/revapi.yml | 39 +++++++++------------------------------ 1 file changed, 9 insertions(+), 30 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index 95f274c6d9e8..cd4afe6fdc8d 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -1370,6 +1370,14 @@ acceptedBreaks: new: "class org.apache.iceberg.encryption.EncryptingFileIO" justification: "New method for Manifest List reading" org.apache.iceberg:iceberg-core: + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.avro.SupportsIndexProjection" + new: "class org.apache.iceberg.avro.SupportsIndexProjection" + justification: "Serialization across versions is not guaranteed" + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.hadoop.SerializableConfiguration" + new: "class org.apache.iceberg.hadoop.SerializableConfiguration" + justification: "Serialization across versions is not guaranteed" - code: "java.class.noLongerInheritsFromClass" old: "class org.apache.iceberg.rest.auth.OAuth2Manager" new: "class org.apache.iceberg.rest.auth.OAuth2Manager" @@ -1408,41 +1416,12 @@ acceptedBreaks: \ java.io.IOException" justification: "Removing deprecated code for 1.11.0" - code: "java.method.removed" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map,\ - \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" - - code: "java.method.numberOfParametersChanged" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ - \ throws java.io.IOException" - new: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map,\ - \ java.lang.String, java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ - \ throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" - - code: "java.element.noLongerDeprecated" old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ \ throws java.io.IOException" - new: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ - \ int, java.util.Map,\ - \ java.lang.String, java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ - \ throws java.io.IOException" - justification: "Position delete files are now rewritten inline during manifest\ - \ writing to fix file_size_in_bytes" + justification: "Removing deprecated code for 1.11.0" - code: "java.method.removed" old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" justification: "Removing deprecated code for 1.11.0" From 669e4f4e397e86a7fc4a3b45f3a618eba6e1d29a Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Wed, 15 Apr 2026 14:06:06 -0400 Subject: [PATCH 08/17] Address PR feedback. --- .../apache/iceberg/RewriteTablePathUtil.java | 23 +++++++++++++------ .../actions/RewriteTablePathSparkAction.java | 8 +++++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index bf9034b55e9b..dabc749a3bd5 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -465,14 +465,15 @@ private static RewriteResult writeDeleteFileEntry( // duplicates across manifests simply overwrite with identical content. String stagingPath = stagingPath(file.location(), sourcePrefix, stagingLocation); OutputFile outputFile = io.newOutputFile(stagingPath); + long actualSize; try { - rewritePositionDeleteFile( - file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); + actualSize = + rewritePositionDeleteFile( + file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); } catch (IOException e) { throw new UncheckedIOException( "Failed to rewrite position delete file " + file.location(), e); } - long actualSize = io.newInputFile(stagingPath).getLength(); DeleteFile posDeleteFile = newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, actualSize); appendEntryWithFile(entry, writer, posDeleteFile); @@ -629,8 +630,9 @@ PositionDeleteWriter writer( * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix to replace it * @param posDeleteReaderWriter class to read and write position delete files + * @return actual file size in bytes after rewriting */ - public static void rewritePositionDeleteFile( + public static long rewritePositionDeleteFile( DeleteFile deleteFile, OutputFile outputFile, FileIO io, @@ -647,8 +649,7 @@ public static void rewritePositionDeleteFile( // DV files (Puffin format for v3+) need special handling to rewrite internal blob metadata if (ContentFileUtil.isDV(deleteFile)) { - rewriteDVFile(deleteFile, outputFile, io, sourcePrefix, targetPrefix); - return; + return rewriteDVFile(deleteFile, outputFile, io, sourcePrefix, targetPrefix); } // For non-DV position delete files (v2), rewrite using the reader/writer @@ -677,9 +678,15 @@ record = recordIt.next(); writer.write(newPositionDeleteRecord(record, sourcePrefix, targetPrefix)); } } + + writer.close(); + return writer.length(); } } } + + // Empty delete file — no records written + return 0; } /** @@ -691,7 +698,7 @@ record = recordIt.next(); * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix to replace it */ - private static void rewriteDVFile( + private static long rewriteDVFile( DeleteFile deleteFile, OutputFile outputFile, FileIO io, @@ -730,6 +737,8 @@ private static void rewriteDVFile( try (PuffinWriter writer = Puffin.write(outputFile).createdBy(IcebergBuild.fullVersion()).build()) { rewrittenBlobs.forEach(writer::write); + writer.close(); + return writer.length(); } } diff --git a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index a6bac705b86a..f18e776a56b1 100644 --- a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -72,6 +72,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.JobGroupInfo; import org.apache.iceberg.spark.source.SerializableTableWithSize; +import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; import org.apache.spark.api.java.function.MapFunction; @@ -311,12 +312,15 @@ private Result rebuildMetadata() { RewriteContentFileResult rewriteManifestResult = rewriteManifests(deltaSnapshots, endMetadata, metaFiles); + // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same + // position delete file can appear in multiple manifests. int rewrittenDeleteFilesCount = (int) rewriteManifestResult.toRewrite().stream() .filter(e -> e instanceof DeleteFile) - .distinct() - .count(); + .map(e -> (DeleteFile) e) + .collect(Collectors.toCollection(DeleteFileSet::create)) + .size(); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() From 78107a1fbeff3945e3449b5849067a561e05ab76 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Wed, 15 Apr 2026 14:11:13 -0400 Subject: [PATCH 09/17] Update revapi. Used ./gradlew :iceberg-core:revapiAcceptAllBreaks --justification "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter for inline position delete rewriting; rewritePositionDeleteFile now returns file size to avoid getLength HEAD call". --- .palantir/revapi.yml | 269 ++++++++++++++++++++++++++----------------- 1 file changed, 164 insertions(+), 105 deletions(-) diff --git a/.palantir/revapi.yml b/.palantir/revapi.yml index cd4afe6fdc8d..4260638cfca4 100644 --- a/.palantir/revapi.yml +++ b/.palantir/revapi.yml @@ -404,6 +404,170 @@ acceptedBreaks: old: "method org.apache.iceberg.orc.ORC.WriteBuilder org.apache.iceberg.orc.ORC.WriteBuilder::config(java.lang.String,\ \ java.lang.String)" justification: "Removing deprecations for 1.2.0" + "1.10.0": + org.apache.iceberg:iceberg-api: + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.encryption.EncryptingFileIO" + new: "class org.apache.iceberg.encryption.EncryptingFileIO" + justification: "New method for Manifest List reading" + org.apache.iceberg:iceberg-core: + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.avro.SupportsIndexProjection" + new: "class org.apache.iceberg.avro.SupportsIndexProjection" + justification: "Serialization across versions is not guaranteed" + - code: "java.class.defaultSerializationChanged" + old: "class org.apache.iceberg.hadoop.SerializableConfiguration" + new: "class org.apache.iceberg.hadoop.SerializableConfiguration" + justification: "Serialization across versions is not guaranteed" + - code: "java.class.noLongerInheritsFromClass" + old: "class org.apache.iceberg.rest.auth.OAuth2Manager" + new: "class org.apache.iceberg.rest.auth.OAuth2Manager" + justification: "Removing deprecations for 1.11.0" + - code: "java.class.nowImplementsInterface" + old: "class org.apache.iceberg.rest.auth.OAuth2Manager" + new: "class org.apache.iceberg.rest.auth.OAuth2Manager" + justification: "Removing deprecations for 1.11.0" + - code: "java.class.removed" + old: "class org.apache.iceberg.PartitionStatsUtil" + justification: "Removing deprecated code for 1.11.0" + - code: "java.class.removed" + old: "class org.apache.iceberg.rest.auth.RefreshingAuthManager" + justification: "Removing deprecations for 1.11.0" + - code: "java.element.noLongerDeprecated" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + new: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ + \ throws java.io.IOException" + justification: "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter\ + \ for inline position delete rewriting; rewritePositionDeleteFile now returns\ + \ file size to avoid getLength HEAD call" + - code: "java.field.constantValueChanged" + old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" + new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" + justification: "Plan API is table scoped and path constant value should include\ + \ namespace. No actual breakage because it never worked before with incorrect\ + \ value." + - code: "java.field.constantValueChanged" + old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" + new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" + justification: "Plan API is table scoped and path constant value should include\ + \ namespace. No actual breakage because it never worked before with incorrect\ + \ value." + - code: "java.field.constantValueChanged" + old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" + new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" + justification: "Plan API is table scoped and path constant value should include\ + \ namespace. No actual breakage because it never worked before with incorrect\ + \ value." + - code: "java.method.numberOfParametersChanged" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + new: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ + \ throws java.io.IOException" + justification: "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter\ + \ for inline position delete rewriting; rewritePositionDeleteFile now returns\ + \ file size to avoid getLength HEAD call" + - code: "java.method.removed" + old: "method java.lang.String org.apache.iceberg.RewriteTablePathUtil::stagingPath(java.lang.String,\ + \ java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDataManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String) throws\ + \ java.io.IOException" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ java.util.Set, org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO,\ + \ int, java.util.Map,\ + \ java.lang.String, java.lang.String, java.lang.String) throws java.io.IOException" + justification: "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter\ + \ for inline position delete rewriting; rewritePositionDeleteFile now returns\ + \ file size to avoid getLength HEAD call" + - code: "java.method.removed" + old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ + \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ + \ throws java.io.IOException" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.TableMetadata org.apache.iceberg.TableMetadataParser::read(org.apache.iceberg.io.FileIO,\ + \ org.apache.iceberg.io.InputFile)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.encryption.EncryptionManager org.apache.iceberg.encryption.EncryptionUtil::createEncryptionManager(java.util.Map, org.apache.iceberg.encryption.KeyManagementClient)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String,\ + \ java.lang.String, java.lang.String, java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String,\ + \ java.lang.String, java.lang.String, java.lang.String, java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.removed" + old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ + \ java.util.Map, java.lang.String, java.lang.String,\ + \ java.lang.String)" + justification: "Removing deprecated code for 1.11.0" + - code: "java.method.returnTypeChanged" + old: "method void org.apache.iceberg.RewriteTablePathUtil::rewritePositionDeleteFile(org.apache.iceberg.DeleteFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, org.apache.iceberg.PartitionSpec,\ + \ java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ + \ throws java.io.IOException" + new: "method long org.apache.iceberg.RewriteTablePathUtil::rewritePositionDeleteFile(org.apache.iceberg.DeleteFile,\ + \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, org.apache.iceberg.PartitionSpec,\ + \ java.lang.String, java.lang.String, org.apache.iceberg.RewriteTablePathUtil.PositionDeleteReaderWriter)\ + \ throws java.io.IOException" + justification: "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter\ + \ for inline position delete rewriting; rewritePositionDeleteFile now returns\ + \ file size to avoid getLength HEAD call" + - code: "java.method.visibilityReduced" + old: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" + new: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" + justification: "Changing deprecated code" + - code: "java.method.visibilityReduced" + old: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" + new: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" + justification: "Changing deprecated code" + - code: "java.method.visibilityReduced" + old: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile,\ + \ org.apache.iceberg.Snapshot)" + new: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile,\ + \ org.apache.iceberg.Snapshot)" + justification: "Changing deprecated code" + org.apache.iceberg:iceberg-data: + - code: "java.class.removed" + old: "class org.apache.iceberg.data.PartitionStatsHandler" + justification: "Removing deprecated code for 1.11.0" "1.2.0": org.apache.iceberg:iceberg-api: - code: "java.field.constantValueChanged" @@ -1363,111 +1527,6 @@ acceptedBreaks: old: "method org.apache.iceberg.parquet.ParquetValueWriters.StructWriter\ \ org.apache.iceberg.data.parquet.GenericParquetWriter::createStructWriter(java.util.List>)" justification: "Removing deprecations for 1.10.0" - "1.10.0": - org.apache.iceberg:iceberg-api: - - code: "java.class.defaultSerializationChanged" - old: "class org.apache.iceberg.encryption.EncryptingFileIO" - new: "class org.apache.iceberg.encryption.EncryptingFileIO" - justification: "New method for Manifest List reading" - org.apache.iceberg:iceberg-core: - - code: "java.class.defaultSerializationChanged" - old: "class org.apache.iceberg.avro.SupportsIndexProjection" - new: "class org.apache.iceberg.avro.SupportsIndexProjection" - justification: "Serialization across versions is not guaranteed" - - code: "java.class.defaultSerializationChanged" - old: "class org.apache.iceberg.hadoop.SerializableConfiguration" - new: "class org.apache.iceberg.hadoop.SerializableConfiguration" - justification: "Serialization across versions is not guaranteed" - - code: "java.class.noLongerInheritsFromClass" - old: "class org.apache.iceberg.rest.auth.OAuth2Manager" - new: "class org.apache.iceberg.rest.auth.OAuth2Manager" - justification: "Removing deprecations for 1.11.0" - - code: "java.class.nowImplementsInterface" - old: "class org.apache.iceberg.rest.auth.OAuth2Manager" - new: "class org.apache.iceberg.rest.auth.OAuth2Manager" - justification: "Removing deprecations for 1.11.0" - - code: "java.class.removed" - old: "class org.apache.iceberg.rest.auth.RefreshingAuthManager" - justification: "Removing deprecations for 1.11.0" - - code: "java.field.constantValueChanged" - old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" - new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN" - justification: "Plan API is table scoped and path constant value should include namespace. No actual breakage because it never worked before with incorrect value." - - code: "java.field.constantValueChanged" - old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" - new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_SUBMIT" - justification: "Plan API is table scoped and path constant value should include namespace. No actual breakage because it never worked before with incorrect value." - - code: "java.field.constantValueChanged" - old: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" - new: "field org.apache.iceberg.rest.ResourcePaths.V1_TABLE_SCAN_PLAN_TASKS" - justification: "Plan API is table scoped and path constant value should include namespace. No actual breakage because it never worked before with incorrect value." - - code: "java.class.removed" - old: "class org.apache.iceberg.PartitionStatsUtil" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method java.lang.String org.apache.iceberg.RewriteTablePathUtil::stagingPath(java.lang.String,\ - \ java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDataManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String) throws\ - \ java.io.IOException" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.RewriteTablePathUtil.RewriteResult\ - \ org.apache.iceberg.RewriteTablePathUtil::rewriteDeleteManifest(org.apache.iceberg.ManifestFile,\ - \ org.apache.iceberg.io.OutputFile, org.apache.iceberg.io.FileIO, int, java.util.Map, java.lang.String, java.lang.String, java.lang.String)\ - \ throws java.io.IOException" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.TableMetadata org.apache.iceberg.TableMetadataParser::read(org.apache.iceberg.io.FileIO,\ - \ org.apache.iceberg.io.InputFile)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.encryption.EncryptionManager org.apache.iceberg.encryption.EncryptionUtil::createEncryptionManager(java.util.Map, org.apache.iceberg.encryption.KeyManagementClient)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String,\ - \ java.lang.String, java.lang.String, java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::exchangeToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String,\ - \ java.lang.String, java.lang.String, java.lang.String, java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.removed" - old: "method org.apache.iceberg.rest.responses.OAuthTokenResponse org.apache.iceberg.rest.auth.OAuth2Util::fetchToken(org.apache.iceberg.rest.RESTClient,\ - \ java.util.Map, java.lang.String, java.lang.String,\ - \ java.lang.String)" - justification: "Removing deprecated code for 1.11.0" - - code: "java.method.visibilityReduced" - old: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile, org.apache.iceberg.Snapshot)" - new: "method void org.apache.iceberg.PartitionStats::liveEntry(org.apache.iceberg.ContentFile, org.apache.iceberg.Snapshot)" - justification: "Changing deprecated code" - - code: "java.method.visibilityReduced" - old: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" - new: "method void org.apache.iceberg.PartitionStats::appendStats(org.apache.iceberg.PartitionStats)" - justification: "Changing deprecated code" - - code: "java.method.visibilityReduced" - old: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" - new: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" - justification: "Changing deprecated code" - org.apache.iceberg:iceberg-data: - - code: "java.class.removed" - old: "class org.apache.iceberg.data.PartitionStatsHandler" - justification: "Removing deprecated code for 1.11.0" apache-iceberg-0.14.0: org.apache.iceberg:iceberg-api: - code: "java.class.defaultSerializationChanged" From 57b3c43a9f6a0d45ad1abb848005eb5bba053416 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 24 Apr 2026 15:38:25 -0400 Subject: [PATCH 10/17] Create Spark 3.4, 3.5, 4.0 versions. --- .../actions/RewriteTablePathSparkAction.java | 77 ++++++------------- .../actions/RewriteTablePathSparkAction.java | 77 ++++++------------- .../actions/RewriteTablePathSparkAction.java | 77 ++++++------------- 3 files changed, 66 insertions(+), 165 deletions(-) diff --git a/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index aedb25e4a4a6..7fb1cd8e9019 100644 --- a/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -75,7 +75,6 @@ import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; -import org.apache.spark.api.java.function.ForeachFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -313,18 +312,20 @@ private Result rebuildMetadata() { RewriteContentFileResult rewriteManifestResult = rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - // rebuild position delete files - Set deleteFiles = - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)); - rewritePositionDeletes(deleteFiles); + // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same + // position delete file can appear in multiple manifests. + int rewrittenDeleteFilesCount = + (int) + rewriteManifestResult.toRewrite().stream() + .filter(e -> e instanceof DeleteFile) + .map(e -> (DeleteFile) e) + .collect(Collectors.toCollection(DeleteFileSet::create)) + .size(); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(deleteFiles.size()) + .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -572,6 +573,8 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); + return manifestDS .repartition(toRewrite.size()) .map( @@ -581,7 +584,8 @@ private RewriteContentFileResult rewriteManifests( stagingDir, tableMetadata.formatVersion(), sourcePrefix, - targetPrefix), + targetPrefix, + posDeleteReaderWriter), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -594,7 +598,8 @@ private static MapFunction toManifests( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -619,7 +624,8 @@ private static MapFunction toManifests( stagingLocation, format, sourcePrefix, - targetPrefix)); + targetPrefix, + posDeleteReaderWriter)); break; default: throw new UnsupportedOperationException( @@ -665,7 +671,8 @@ private static RewriteResult writeDeleteManifest( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -682,29 +689,13 @@ private static RewriteResult writeDeleteManifest( specsById, sourcePrefix, targetPrefix, - stagingLocation); + stagingLocation, + posDeleteReaderWriter); } catch (IOException e) { throw new RuntimeIOException(e); } } - private void rewritePositionDeletes(Set toRewrite) { - if (toRewrite.isEmpty()) { - return; - } - - Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDs = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); - - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); - deleteFileDs - .repartition(toRewrite.size()) - .foreach( - rewritePositionDelete( - tableBroadcast(), sourcePrefix, targetPrefix, stagingDir, posDeleteReaderWriter)); - } - private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( @@ -724,30 +715,6 @@ public PositionDeleteWriter writer( } } - private ForeachFunction rewritePositionDelete( - Broadcast
tableArg, - String sourcePrefixArg, - String targetPrefixArg, - String stagingLocationArg, - PositionDeleteReaderWriter posDeleteReaderWriter) { - return deleteFile -> { - FileIO io = tableArg.getValue().io(); - String newPath = - RewriteTablePathUtil.stagingPath( - deleteFile.location(), sourcePrefixArg, stagingLocationArg); - OutputFile outputFile = io.newOutputFile(newPath); - PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); - RewriteTablePathUtil.rewritePositionDeleteFile( - deleteFile, - outputFile, - io, - spec, - sourcePrefixArg, - targetPrefixArg, - posDeleteReaderWriter); - }; - } - private static CloseableIterable positionDeletesReader( InputFile inputFile, FileFormat format, PartitionSpec spec) { return FormatModelRegistry.readBuilder(format, Record.class, inputFile) diff --git a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index aedb25e4a4a6..7fb1cd8e9019 100644 --- a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -75,7 +75,6 @@ import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; -import org.apache.spark.api.java.function.ForeachFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -313,18 +312,20 @@ private Result rebuildMetadata() { RewriteContentFileResult rewriteManifestResult = rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - // rebuild position delete files - Set deleteFiles = - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)); - rewritePositionDeletes(deleteFiles); + // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same + // position delete file can appear in multiple manifests. + int rewrittenDeleteFilesCount = + (int) + rewriteManifestResult.toRewrite().stream() + .filter(e -> e instanceof DeleteFile) + .map(e -> (DeleteFile) e) + .collect(Collectors.toCollection(DeleteFileSet::create)) + .size(); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(deleteFiles.size()) + .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -572,6 +573,8 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); + return manifestDS .repartition(toRewrite.size()) .map( @@ -581,7 +584,8 @@ private RewriteContentFileResult rewriteManifests( stagingDir, tableMetadata.formatVersion(), sourcePrefix, - targetPrefix), + targetPrefix, + posDeleteReaderWriter), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -594,7 +598,8 @@ private static MapFunction toManifests( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -619,7 +624,8 @@ private static MapFunction toManifests( stagingLocation, format, sourcePrefix, - targetPrefix)); + targetPrefix, + posDeleteReaderWriter)); break; default: throw new UnsupportedOperationException( @@ -665,7 +671,8 @@ private static RewriteResult writeDeleteManifest( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -682,29 +689,13 @@ private static RewriteResult writeDeleteManifest( specsById, sourcePrefix, targetPrefix, - stagingLocation); + stagingLocation, + posDeleteReaderWriter); } catch (IOException e) { throw new RuntimeIOException(e); } } - private void rewritePositionDeletes(Set toRewrite) { - if (toRewrite.isEmpty()) { - return; - } - - Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDs = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); - - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); - deleteFileDs - .repartition(toRewrite.size()) - .foreach( - rewritePositionDelete( - tableBroadcast(), sourcePrefix, targetPrefix, stagingDir, posDeleteReaderWriter)); - } - private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( @@ -724,30 +715,6 @@ public PositionDeleteWriter writer( } } - private ForeachFunction rewritePositionDelete( - Broadcast
tableArg, - String sourcePrefixArg, - String targetPrefixArg, - String stagingLocationArg, - PositionDeleteReaderWriter posDeleteReaderWriter) { - return deleteFile -> { - FileIO io = tableArg.getValue().io(); - String newPath = - RewriteTablePathUtil.stagingPath( - deleteFile.location(), sourcePrefixArg, stagingLocationArg); - OutputFile outputFile = io.newOutputFile(newPath); - PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); - RewriteTablePathUtil.rewritePositionDeleteFile( - deleteFile, - outputFile, - io, - spec, - sourcePrefixArg, - targetPrefixArg, - posDeleteReaderWriter); - }; - } - private static CloseableIterable positionDeletesReader( InputFile inputFile, FileFormat format, PartitionSpec spec) { return FormatModelRegistry.readBuilder(format, Record.class, inputFile) diff --git a/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index aedb25e4a4a6..7fb1cd8e9019 100644 --- a/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -75,7 +75,6 @@ import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; -import org.apache.spark.api.java.function.ForeachFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -313,18 +312,20 @@ private Result rebuildMetadata() { RewriteContentFileResult rewriteManifestResult = rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - // rebuild position delete files - Set deleteFiles = - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)); - rewritePositionDeletes(deleteFiles); + // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same + // position delete file can appear in multiple manifests. + int rewrittenDeleteFilesCount = + (int) + rewriteManifestResult.toRewrite().stream() + .filter(e -> e instanceof DeleteFile) + .map(e -> (DeleteFile) e) + .collect(Collectors.toCollection(DeleteFileSet::create)) + .size(); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(deleteFiles.size()) + .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -572,6 +573,8 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); + return manifestDS .repartition(toRewrite.size()) .map( @@ -581,7 +584,8 @@ private RewriteContentFileResult rewriteManifests( stagingDir, tableMetadata.formatVersion(), sourcePrefix, - targetPrefix), + targetPrefix, + posDeleteReaderWriter), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -594,7 +598,8 @@ private static MapFunction toManifests( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -619,7 +624,8 @@ private static MapFunction toManifests( stagingLocation, format, sourcePrefix, - targetPrefix)); + targetPrefix, + posDeleteReaderWriter)); break; default: throw new UnsupportedOperationException( @@ -665,7 +671,8 @@ private static RewriteResult writeDeleteManifest( String stagingLocation, int format, String sourcePrefix, - String targetPrefix) { + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -682,29 +689,13 @@ private static RewriteResult writeDeleteManifest( specsById, sourcePrefix, targetPrefix, - stagingLocation); + stagingLocation, + posDeleteReaderWriter); } catch (IOException e) { throw new RuntimeIOException(e); } } - private void rewritePositionDeletes(Set toRewrite) { - if (toRewrite.isEmpty()) { - return; - } - - Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDs = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); - - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); - deleteFileDs - .repartition(toRewrite.size()) - .foreach( - rewritePositionDelete( - tableBroadcast(), sourcePrefix, targetPrefix, stagingDir, posDeleteReaderWriter)); - } - private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( @@ -724,30 +715,6 @@ public PositionDeleteWriter writer( } } - private ForeachFunction rewritePositionDelete( - Broadcast
tableArg, - String sourcePrefixArg, - String targetPrefixArg, - String stagingLocationArg, - PositionDeleteReaderWriter posDeleteReaderWriter) { - return deleteFile -> { - FileIO io = tableArg.getValue().io(); - String newPath = - RewriteTablePathUtil.stagingPath( - deleteFile.location(), sourcePrefixArg, stagingLocationArg); - OutputFile outputFile = io.newOutputFile(newPath); - PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); - RewriteTablePathUtil.rewritePositionDeleteFile( - deleteFile, - outputFile, - io, - spec, - sourcePrefixArg, - targetPrefixArg, - posDeleteReaderWriter); - }; - } - private static CloseableIterable positionDeletesReader( InputFile inputFile, FileFormat format, PartitionSpec spec) { return FormatModelRegistry.readBuilder(format, Record.class, inputFile) From e66e0b78c270f931594d08d5df4d17227277aac7 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Wed, 6 May 2026 10:04:46 -0400 Subject: [PATCH 11/17] Fix after upmerge with main. --- .../iceberg/TestRewriteTablePathUtil.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java b/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java index bedd8dd66d71..cd02776735aa 100644 --- a/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java +++ b/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java @@ -24,6 +24,11 @@ import java.io.IOException; import java.util.Set; +import org.apache.iceberg.data.Record; +import org.apache.iceberg.deletes.PositionDeleteWriter; +import org.apache.iceberg.io.CloseableIterable; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtendWith; @@ -277,7 +282,24 @@ public void testRewritingMultiplePositionDeleteEntriesWithinManifestFile() throw table.specs(), sourcePrefix, targetPrefix, - stagingDir); + stagingDir, + new RewriteTablePathUtil.PositionDeleteReaderWriter() { + @Override + public CloseableIterable reader( + InputFile inputFile, FileFormat format, PartitionSpec spec) { + return CloseableIterable.empty(); + } + + @Override + public PositionDeleteWriter writer( + OutputFile outputFile, + FileFormat format, + PartitionSpec spec, + StructLike partition, + Schema rowSchema) { + throw new UnsupportedOperationException(); + } + }); assertThat(deleteFileRewriteResult.toRewrite()).hasSize(2); } From 2b54f869bd0ad264eeca69d96e739498b778dd1b Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Tue, 12 May 2026 09:53:16 -0400 Subject: [PATCH 12/17] Dedupe concurrent rewrites of the same position delete file across manifest tasks to avoid FileAlreadyExistsException on the shared staging path. --- .../apache/iceberg/RewriteTablePathUtil.java | 63 ++++++++++++++----- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index dabc749a3bd5..1a3ea023fe96 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -28,6 +28,7 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.StreamSupport; +import org.apache.hadoop.fs.FileAlreadyExistsException; import org.apache.iceberg.data.Record; import org.apache.iceberg.deletes.PositionDelete; import org.apache.iceberg.deletes.PositionDeleteWriter; @@ -394,6 +395,7 @@ public static RewriteResult rewriteDeleteManifest( PositionDeleteReaderWriter posDeleteReaderWriter) throws IOException { PartitionSpec spec = specsById.get(manifestFile.partitionSpecId()); + Map rewrittenSizesBySourcePath = Maps.newHashMap(); try (ManifestWriter writer = ManifestFiles.writeDeleteManifest(format, spec, outputFile, manifestFile.snapshotId()); ManifestReader reader = @@ -411,7 +413,8 @@ public static RewriteResult rewriteDeleteManifest( stagingLocation, writer, io, - posDeleteReaderWriter)) + posDeleteReaderWriter, + rewrittenSizesBySourcePath)) .reduce(new RewriteResult<>(), RewriteResult::append); } } @@ -453,7 +456,8 @@ private static RewriteResult writeDeleteFileEntry( String stagingLocation, ManifestWriter writer, FileIO io, - PositionDeleteReaderWriter posDeleteReaderWriter) { + PositionDeleteReaderWriter posDeleteReaderWriter, + Map rewrittenSizesBySourcePath) { DeleteFile file = entry.file(); RewriteResult result = new RewriteResult<>(); @@ -461,19 +465,23 @@ private static RewriteResult writeDeleteFileEntry( switch (file.content()) { case POSITION_DELETES: // Rewrite inline so the manifest records the actual file size, which changes because - // embedded data file paths are rewritten. The staging path is deterministic, so - // duplicates across manifests simply overwrite with identical content. - String stagingPath = stagingPath(file.location(), sourcePrefix, stagingLocation); - OutputFile outputFile = io.newOutputFile(stagingPath); - long actualSize; - try { - actualSize = - rewritePositionDeleteFile( - file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); - } catch (IOException e) { - throw new UncheckedIOException( - "Failed to rewrite position delete file " + file.location(), e); - } + // embedded data file paths are rewritten. The same source path may be referenced by + // multiple entries (here or in another task processing a different manifest); rewriting + // is deterministic, so cache the size and recover from cross-task collisions on read. + String sourcePath = file.location(); + String stagingPath = stagingPath(sourcePath, sourcePrefix, stagingLocation); + long actualSize = + rewrittenSizesBySourcePath.computeIfAbsent( + sourcePath, + ignored -> + rewriteOrReuseStagedPositionDeleteFile( + file, + stagingPath, + io, + spec, + sourcePrefix, + targetPrefix, + posDeleteReaderWriter)); DeleteFile posDeleteFile = newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, actualSize); appendEntryWithFile(entry, writer, posDeleteFile); @@ -502,6 +510,31 @@ private static RewriteResult writeDeleteFileEntry( } } + private static long rewriteOrReuseStagedPositionDeleteFile( + DeleteFile file, + String stagingPath, + FileIO io, + PartitionSpec spec, + String sourcePrefix, + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) { + OutputFile outputFile = io.newOutputFile(stagingPath); + try { + return rewritePositionDeleteFile( + file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); + } catch (IOException e) { + throw new UncheckedIOException( + "Failed to rewrite position delete file " + file.location(), e); + } catch (UncheckedIOException e) { + // Another task in this Spark job already staged this file. Rewriting is deterministic, so + // its content (and therefore length) match what this task would have produced. + if (e.getCause() instanceof FileAlreadyExistsException) { + return io.newInputFile(stagingPath).getLength(); + } + throw e; + } + } + private static > void appendEntryWithFile( ManifestEntry entry, ManifestWriter writer, F file) { From 81469d5dc006bd45eed7d64e371df8e12af249f2 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Thu, 28 May 2026 12:30:24 -0400 Subject: [PATCH 13/17] Address PR comments. --- .../apache/iceberg/RewriteTablePathUtil.java | 152 +++++++++++++++--- 1 file changed, 132 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index 7555d94f742f..8946349c3294 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -26,9 +26,9 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import java.util.stream.Collectors; import java.util.stream.StreamSupport; -import org.apache.hadoop.fs.FileAlreadyExistsException; import org.apache.iceberg.data.Record; import org.apache.iceberg.deletes.PositionDelete; import org.apache.iceberg.deletes.PositionDeleteWriter; @@ -364,6 +364,58 @@ public static RewriteResult rewriteDataManifest( } } + /** + * Rewrite a delete manifest, replacing path references. + * + * @param manifestFile source delete manifest to rewrite + * @param snapshotIds snapshot ids for filtering returned delete manifest entries + * @param outputFile output file to rewrite manifest file to + * @param io file io + * @param format format of the manifest file + * @param specsById map of partition specs by id + * @param sourcePrefix source prefix that will be replaced + * @param targetPrefix target prefix that will replace it + * @param stagingLocation staging location for rewritten files (referred delete file will be + * rewritten here) + * @return a copy plan of content files in the manifest that was rewritten + * @deprecated since 1.11.0, will be removed in 1.12.0; use the overload that accepts a {@link + * PositionDeleteReaderWriter}. This overload does not rewrite position delete file content, + * so the manifest's {@code file_size_in_bytes} can be inconsistent with the rewritten file + * size on disk. + */ + @Deprecated + public static RewriteResult rewriteDeleteManifest( + ManifestFile manifestFile, + Set snapshotIds, + OutputFile outputFile, + FileIO io, + int format, + Map specsById, + String sourcePrefix, + String targetPrefix, + String stagingLocation) + throws IOException { + PartitionSpec spec = specsById.get(manifestFile.partitionSpecId()); + try (ManifestWriter writer = + ManifestFiles.writeDeleteManifest(format, spec, outputFile, manifestFile.snapshotId()); + ManifestReader reader = + ManifestFiles.readDeleteManifest(manifestFile, io, specsById) + .select(Arrays.asList("*"))) { + return StreamSupport.stream(reader.entries().spliterator(), false) + .map( + entry -> + writeDeleteFileEntry( + entry, + snapshotIds, + spec, + sourcePrefix, + targetPrefix, + stagingLocation, + writer)) + .reduce(new RewriteResult<>(), RewriteResult::append); + } + } + /** * Rewrite a delete manifest, replacing path references. * @@ -395,6 +447,9 @@ public static RewriteResult rewriteDeleteManifest( PositionDeleteReaderWriter posDeleteReaderWriter) throws IOException { PartitionSpec spec = specsById.get(manifestFile.partitionSpecId()); + // Scope rewritten files under a per-task subdirectory so concurrent tasks rewriting the same + // source delete file (referenced from different manifests) do not collide on a shared path. + String taskStagingLocation = combinePaths(stagingLocation, UUID.randomUUID().toString()); Map rewrittenSizesBySourcePath = Maps.newHashMap(); try (ManifestWriter writer = ManifestFiles.writeDeleteManifest(format, spec, outputFile, manifestFile.snapshotId()); @@ -404,13 +459,13 @@ public static RewriteResult rewriteDeleteManifest( return StreamSupport.stream(reader.entries().spliterator(), false) .map( entry -> - writeDeleteFileEntry( + writeDeleteFileEntryWithRewrite( entry, snapshotIds, spec, sourcePrefix, targetPrefix, - stagingLocation, + taskStagingLocation, writer, io, posDeleteReaderWriter, @@ -447,7 +502,7 @@ private static RewriteResult writeDataFileEntry( return result; } - private static RewriteResult writeDeleteFileEntry( + private static RewriteResult writeDeleteFileEntryWithRewrite( ManifestEntry entry, Set snapshotIds, PartitionSpec spec, @@ -465,16 +520,15 @@ private static RewriteResult writeDeleteFileEntry( switch (file.content()) { case POSITION_DELETES: // Rewrite inline so the manifest records the actual file size, which changes because - // embedded data file paths are rewritten. The same source path may be referenced by - // multiple entries (here or in another task processing a different manifest); rewriting - // is deterministic, so cache the size and recover from cross-task collisions on read. + // embedded data file paths are rewritten. Same source path may appear in multiple entries + // within this manifest, so cache the rewritten size to avoid redundant work. String sourcePath = file.location(); String stagingPath = stagingPath(sourcePath, sourcePrefix, stagingLocation); long actualSize = rewrittenSizesBySourcePath.computeIfAbsent( sourcePath, ignored -> - rewriteOrReuseStagedPositionDeleteFile( + rewriteStagedPositionDeleteFile( file, stagingPath, io, @@ -510,7 +564,53 @@ private static RewriteResult writeDeleteFileEntry( } } - private static long rewriteOrReuseStagedPositionDeleteFile( + private static RewriteResult writeDeleteFileEntry( + ManifestEntry entry, + Set snapshotIds, + PartitionSpec spec, + String sourcePrefix, + String targetPrefix, + String stagingLocation, + ManifestWriter writer) { + + DeleteFile file = entry.file(); + RewriteResult result = new RewriteResult<>(); + + switch (file.content()) { + case POSITION_DELETES: + DeleteFile posDeleteFile = newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix); + appendEntryWithFile(entry, writer, posDeleteFile); + // keep the following entries in metadata but exclude them from copyPlan + // 1) deleted position delete files + // 2) entries not changed by snapshotIds + if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) { + result + .copyPlan() + .add( + Pair.of( + stagingPath(file.location(), sourcePrefix, stagingLocation), + posDeleteFile.location())); + } + result.toRewrite().add(file.copy()); + return result; + case EQUALITY_DELETES: + DeleteFile eqDeleteFile = newEqualityDeleteEntry(file, spec, sourcePrefix, targetPrefix); + appendEntryWithFile(entry, writer, eqDeleteFile); + // keep the following entries in metadata but exclude them from copyPlan + // 1) deleted equality delete files + // 2) entries not changed by snapshotIds + if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) { + // No need to rewrite equality delete files as they do not contain absolute file paths. + result.copyPlan().add(Pair.of(file.location(), eqDeleteFile.location())); + } + return result; + + default: + throw new UnsupportedOperationException("Unsupported delete file type: " + file.content()); + } + } + + private static long rewriteStagedPositionDeleteFile( DeleteFile file, String stagingPath, FileIO io, @@ -520,18 +620,11 @@ private static long rewriteOrReuseStagedPositionDeleteFile( PositionDeleteReaderWriter posDeleteReaderWriter) { OutputFile outputFile = io.newOutputFile(stagingPath); try { - return rewritePositionDeleteFile( + return rewritePositionDeleteFileReturningLength( file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); } catch (IOException e) { throw new UncheckedIOException( "Failed to rewrite position delete file " + file.location(), e); - } catch (UncheckedIOException e) { - // Another task in this Spark job already staged this file. Rewriting is deterministic, so - // its content (and therefore length) match what this task would have produced. - if (e.getCause() instanceof FileAlreadyExistsException) { - return io.newInputFile(stagingPath).getLength(); - } - throw e; } } @@ -573,6 +666,11 @@ private static DeleteFile newEqualityDeleteEntry( .build(); } + private static DeleteFile newPositionDeleteEntry( + DeleteFile file, PartitionSpec spec, String sourcePrefix, String targetPrefix) { + return newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, file.fileSizeInBytes()); + } + private static DeleteFile newPositionDeleteEntry( DeleteFile file, PartitionSpec spec, @@ -663,9 +761,24 @@ PositionDeleteWriter writer( * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix to replace it * @param posDeleteReaderWriter class to read and write position delete files - * @return actual file size in bytes after rewriting */ - public static long rewritePositionDeleteFile( + public static void rewritePositionDeleteFile( + DeleteFile deleteFile, + OutputFile outputFile, + FileIO io, + PartitionSpec spec, + String sourcePrefix, + String targetPrefix, + PositionDeleteReaderWriter posDeleteReaderWriter) + throws IOException { + rewritePositionDeleteFileReturningLength( + deleteFile, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); + } + + // Returns the actual rewritten file size so callers can record it in the manifest. Kept + // package-private to avoid expanding the public API; the public overload above intentionally + // discards the return value. + static long rewritePositionDeleteFileReturningLength( DeleteFile deleteFile, OutputFile outputFile, FileIO io, @@ -718,7 +831,6 @@ record = recordIt.next(); } } - // Empty delete file — no records written return 0; } From bfd6e650bc3bed1dfed51d5f5b1a47ea058c25ac Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Thu, 28 May 2026 12:37:14 -0400 Subject: [PATCH 14/17] Add test for position delete file in multiple snapshots. --- .../actions/TestRewriteTablePathsAction.java | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index 3b3cf400a133..551339402c7c 100644 --- a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -645,6 +645,74 @@ public void testPositionDeletesDeduplication() throws Exception { .isEqualTo(1); } + // Regression test: when the same position delete file is referenced from manifests in different + // snapshots, each manifest is rewritten by a separate Spark task. Without per-task staging path + // isolation those tasks would collide on a shared path, either failing or recording an + // inconsistent file_size_in_bytes in one of the rewritten manifests. + @TestTemplate + public void testSharedDeleteFileSizeAcrossManifests() throws Exception { + assumeThat(formatVersion) + .as("Format versions 3+ use DVs with different validation rules") + .isEqualTo(2); + + Table tableWithPosDeletes = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithSharedDelete"), + 1, + Map.of(TableProperties.DELETE_DEFAULT_FILE_FORMAT, "parquet")); + + DataFile dataFile = + tableWithPosDeletes + .currentSnapshot() + .addedDataFiles(tableWithPosDeletes.io()) + .iterator() + .next(); + + List> deletes = Lists.newArrayList(Pair.of(dataFile.location(), 0L)); + File deleteFile = + new File( + removePrefix(tableWithPosDeletes.location() + "/data/deeply/nested/deletes.parquet")); + DeleteFile positionDeletes = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(deleteFile.toURI().toString()), + deletes, + formatVersion) + .first(); + + tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); + tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithPosDeletes) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + List deleteManifests = + targetTable.currentSnapshot().deleteManifests(targetTable.io()); + assertThat(deleteManifests) + .as("Expected the shared delete file to be referenced by multiple manifests") + .hasSizeGreaterThanOrEqualTo(2); + for (ManifestFile manifest : deleteManifests) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + } + } + } + } + // Regression test: rewriting delete file paths changes the file size (since the // embedded data file paths may differ in length), but file_size_in_bytes in the rewritten // manifest was not updated. Readers that use file_size_in_bytes to elide a stat() call may From 3ffa40611b3eb8f60e6904ce29aa10b5db1f340f Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 5 Jun 2026 15:33:29 -0400 Subject: [PATCH 15/17] Fix new test setup. --- .../iceberg/spark/actions/TestRewriteTablePathsAction.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index 551339402c7c..8f100232b45c 100644 --- a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -722,7 +722,12 @@ public void testDeleteFileSizeInBytesAfterRewrite() throws Exception { List> deletes = Lists.newArrayList( Pair.of( - table.currentSnapshot().addedDataFiles(table.io()).iterator().next().location(), + SnapshotChanges.builderFor(table) + .build() + .addedDataFiles() + .iterator() + .next() + .location(), 0L)); File file = new File(removePrefix(table.location() + "/data/deeply/nested/deletes.parquet")); From 3ed7b951b8ba5233fcb4992fdab997e87f42542d Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Fri, 12 Jun 2026 11:40:33 -0400 Subject: [PATCH 16/17] Core, Spark: measure rewritten delete file size in a parallel phase to address PR feedback --- .../apache/iceberg/RewriteTablePathUtil.java | 180 ++++++------------ .../iceberg/TestRewriteTablePathUtil.java | 24 +-- .../actions/RewriteTablePathSparkAction.java | 164 ++++++++++++++-- .../actions/RewriteTablePathSparkAction.java | 164 ++++++++++++++-- .../actions/RewriteTablePathSparkAction.java | 163 ++++++++++++++-- .../actions/TestRewriteTablePathsAction.java | 92 ++++++++- 6 files changed, 576 insertions(+), 211 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index 8946349c3294..c56c6cac908f 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -26,7 +26,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.UUID; import java.util.stream.Collectors; import java.util.stream.StreamSupport; import org.apache.iceberg.data.Record; @@ -48,6 +47,7 @@ import org.apache.iceberg.puffin.PuffinReader; import org.apache.iceberg.puffin.PuffinWriter; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; @@ -378,10 +378,10 @@ public static RewriteResult rewriteDataManifest( * @param stagingLocation staging location for rewritten files (referred delete file will be * rewritten here) * @return a copy plan of content files in the manifest that was rewritten - * @deprecated since 1.11.0, will be removed in 1.12.0; use the overload that accepts a {@link - * PositionDeleteReaderWriter}. This overload does not rewrite position delete file content, - * so the manifest's {@code file_size_in_bytes} can be inconsistent with the rewritten file - * size on disk. + * @deprecated since 1.11.0, will be removed in 1.12.0; use the overload that accepts the map of + * rewritten position delete file sizes. This overload records the original {@code + * file_size_in_bytes}, which can be inconsistent with the rewritten file size on disk once + * embedded data file paths change length. */ @Deprecated public static RewriteResult rewriteDeleteManifest( @@ -395,32 +395,26 @@ public static RewriteResult rewriteDeleteManifest( String targetPrefix, String stagingLocation) throws IOException { - PartitionSpec spec = specsById.get(manifestFile.partitionSpecId()); - try (ManifestWriter writer = - ManifestFiles.writeDeleteManifest(format, spec, outputFile, manifestFile.snapshotId()); - ManifestReader reader = - ManifestFiles.readDeleteManifest(manifestFile, io, specsById) - .select(Arrays.asList("*"))) { - return StreamSupport.stream(reader.entries().spliterator(), false) - .map( - entry -> - writeDeleteFileEntry( - entry, - snapshotIds, - spec, - sourcePrefix, - targetPrefix, - stagingLocation, - writer)) - .reduce(new RewriteResult<>(), RewriteResult::append); - } + return rewriteDeleteManifest( + manifestFile, + snapshotIds, + outputFile, + io, + format, + specsById, + sourcePrefix, + targetPrefix, + stagingLocation, + ImmutableMap.of()); } /** * Rewrite a delete manifest, replacing path references. * - *

Position delete files are rewritten inline so that the manifest records the actual file size - * after path rewriting. + *

This is a metadata-only operation: position delete file content is rewritten separately (see + * {@link #rewritePositionDeleteFile}). The actual sizes of those rewritten files are supplied via + * {@code rewrittenDeleteFileSizes} and recorded in the manifest so that {@code + * file_size_in_bytes} stays consistent with the rewritten file on disk. * * @param manifestFile source delete manifest to rewrite * @param snapshotIds snapshot ids for filtering returned delete manifest entries @@ -431,7 +425,8 @@ public static RewriteResult rewriteDeleteManifest( * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix that will replace it * @param stagingLocation staging location for rewritten position delete files - * @param posDeleteReaderWriter reader/writer for position delete files + * @param rewrittenDeleteFileSizes map from source position delete file path to the actual size of + * the rewritten file; entries absent from the map keep their original size * @return a copy plan of content files in the manifest that was rewritten */ public static RewriteResult rewriteDeleteManifest( @@ -444,13 +439,9 @@ public static RewriteResult rewriteDeleteManifest( String sourcePrefix, String targetPrefix, String stagingLocation, - PositionDeleteReaderWriter posDeleteReaderWriter) + Map rewrittenDeleteFileSizes) throws IOException { PartitionSpec spec = specsById.get(manifestFile.partitionSpecId()); - // Scope rewritten files under a per-task subdirectory so concurrent tasks rewriting the same - // source delete file (referenced from different manifests) do not collide on a shared path. - String taskStagingLocation = combinePaths(stagingLocation, UUID.randomUUID().toString()); - Map rewrittenSizesBySourcePath = Maps.newHashMap(); try (ManifestWriter writer = ManifestFiles.writeDeleteManifest(format, spec, outputFile, manifestFile.snapshotId()); ManifestReader reader = @@ -459,17 +450,15 @@ public static RewriteResult rewriteDeleteManifest( return StreamSupport.stream(reader.entries().spliterator(), false) .map( entry -> - writeDeleteFileEntryWithRewrite( + writeDeleteFileEntry( entry, snapshotIds, spec, sourcePrefix, targetPrefix, - taskStagingLocation, + stagingLocation, writer, - io, - posDeleteReaderWriter, - rewrittenSizesBySourcePath)) + rewrittenDeleteFileSizes)) .reduce(new RewriteResult<>(), RewriteResult::append); } } @@ -502,7 +491,7 @@ private static RewriteResult writeDataFileEntry( return result; } - private static RewriteResult writeDeleteFileEntryWithRewrite( + private static RewriteResult writeDeleteFileEntry( ManifestEntry entry, Set snapshotIds, PartitionSpec spec, @@ -510,75 +499,20 @@ private static RewriteResult writeDeleteFileEntryWithRewrite( String targetPrefix, String stagingLocation, ManifestWriter writer, - FileIO io, - PositionDeleteReaderWriter posDeleteReaderWriter, - Map rewrittenSizesBySourcePath) { + Map rewrittenDeleteFileSizes) { DeleteFile file = entry.file(); RewriteResult result = new RewriteResult<>(); switch (file.content()) { case POSITION_DELETES: - // Rewrite inline so the manifest records the actual file size, which changes because - // embedded data file paths are rewritten. Same source path may appear in multiple entries - // within this manifest, so cache the rewritten size to avoid redundant work. - String sourcePath = file.location(); - String stagingPath = stagingPath(sourcePath, sourcePrefix, stagingLocation); - long actualSize = - rewrittenSizesBySourcePath.computeIfAbsent( - sourcePath, - ignored -> - rewriteStagedPositionDeleteFile( - file, - stagingPath, - io, - spec, - sourcePrefix, - targetPrefix, - posDeleteReaderWriter)); + // Rewriting the embedded data file paths changes the file size, so record the actual size + // measured when the file was rewritten. Falls back to the original size for entries whose + // file was not rewritten (e.g. deleted entries that are not copied to the target). + long fileSizeInBytes = + rewrittenDeleteFileSizes.getOrDefault(file.location(), file.fileSizeInBytes()); DeleteFile posDeleteFile = - newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, actualSize); - appendEntryWithFile(entry, writer, posDeleteFile); - // keep the following entries in metadata but exclude them from copyPlan - // 1) deleted position delete files - // 2) entries not changed by snapshotIds - if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) { - result.copyPlan().add(Pair.of(stagingPath, posDeleteFile.location())); - } - result.toRewrite().add(file.copy()); - return result; - case EQUALITY_DELETES: - DeleteFile eqDeleteFile = newEqualityDeleteEntry(file, spec, sourcePrefix, targetPrefix); - appendEntryWithFile(entry, writer, eqDeleteFile); - // keep the following entries in metadata but exclude them from copyPlan - // 1) deleted equality delete files - // 2) entries not changed by snapshotIds - if (entry.isLive() && snapshotIds.contains(entry.snapshotId())) { - // No need to rewrite equality delete files as they do not contain absolute file paths. - result.copyPlan().add(Pair.of(file.location(), eqDeleteFile.location())); - } - return result; - - default: - throw new UnsupportedOperationException("Unsupported delete file type: " + file.content()); - } - } - - private static RewriteResult writeDeleteFileEntry( - ManifestEntry entry, - Set snapshotIds, - PartitionSpec spec, - String sourcePrefix, - String targetPrefix, - String stagingLocation, - ManifestWriter writer) { - - DeleteFile file = entry.file(); - RewriteResult result = new RewriteResult<>(); - - switch (file.content()) { - case POSITION_DELETES: - DeleteFile posDeleteFile = newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix); + newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, fileSizeInBytes); appendEntryWithFile(entry, writer, posDeleteFile); // keep the following entries in metadata but exclude them from copyPlan // 1) deleted position delete files @@ -610,24 +544,6 @@ private static RewriteResult writeDeleteFileEntry( } } - private static long rewriteStagedPositionDeleteFile( - DeleteFile file, - String stagingPath, - FileIO io, - PartitionSpec spec, - String sourcePrefix, - String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { - OutputFile outputFile = io.newOutputFile(stagingPath); - try { - return rewritePositionDeleteFileReturningLength( - file, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); - } catch (IOException e) { - throw new UncheckedIOException( - "Failed to rewrite position delete file " + file.location(), e); - } - } - private static > void appendEntryWithFile( ManifestEntry entry, ManifestWriter writer, F file) { @@ -666,11 +582,6 @@ private static DeleteFile newEqualityDeleteEntry( .build(); } - private static DeleteFile newPositionDeleteEntry( - DeleteFile file, PartitionSpec spec, String sourcePrefix, String targetPrefix) { - return newPositionDeleteEntry(file, spec, sourcePrefix, targetPrefix, file.fileSizeInBytes()); - } - private static DeleteFile newPositionDeleteEntry( DeleteFile file, PartitionSpec spec, @@ -775,10 +686,25 @@ public static void rewritePositionDeleteFile( deleteFile, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); } - // Returns the actual rewritten file size so callers can record it in the manifest. Kept - // package-private to avoid expanding the public API; the public overload above intentionally - // discards the return value. - static long rewritePositionDeleteFileReturningLength( + /** + * Rewrite a position delete file, replacing path references, and return the size of the rewritten + * file. + * + *

The size is measured from the writer after it is closed (rather than via a separate {@code + * getLength()}/HEAD call), so it is accurate even on file systems where the length of an + * in-progress write underreports. Callers record this size as {@code file_size_in_bytes} in the + * rewritten manifest. + * + * @param deleteFile source position delete file to rewrite + * @param outputFile output file to write the rewritten delete file to + * @param io file io + * @param spec spec of delete file + * @param sourcePrefix source prefix that will be replaced + * @param targetPrefix target prefix to replace it + * @param posDeleteReaderWriter class to read and write position delete files + * @return the size in bytes of the rewritten file + */ + public static long rewritePositionDeleteFileReturningLength( DeleteFile deleteFile, OutputFile outputFile, FileIO io, diff --git a/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java b/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java index cd02776735aa..e848865fb21f 100644 --- a/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java +++ b/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java @@ -24,11 +24,7 @@ import java.io.IOException; import java.util.Set; -import org.apache.iceberg.data.Record; -import org.apache.iceberg.deletes.PositionDeleteWriter; -import org.apache.iceberg.io.CloseableIterable; -import org.apache.iceberg.io.InputFile; -import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtendWith; @@ -283,23 +279,7 @@ public void testRewritingMultiplePositionDeleteEntriesWithinManifestFile() throw sourcePrefix, targetPrefix, stagingDir, - new RewriteTablePathUtil.PositionDeleteReaderWriter() { - @Override - public CloseableIterable reader( - InputFile inputFile, FileFormat format, PartitionSpec spec) { - return CloseableIterable.empty(); - } - - @Override - public PositionDeleteWriter writer( - OutputFile outputFile, - FileFormat format, - PartitionSpec spec, - StructLike partition, - Schema rowSchema) { - throw new UnsupportedOperationException(); - } - }); + ImmutableMap.of()); assertThat(deleteFileRewriteResult.toRewrite()).hasSize(2); } diff --git a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index 7fb1cd8e9019..fae1b0170eb7 100644 --- a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -31,9 +32,13 @@ import org.apache.iceberg.ContentFile; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileContent; import org.apache.iceberg.FileFormat; import org.apache.iceberg.HasTableOperations; +import org.apache.iceberg.ManifestContent; import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestFiles; +import org.apache.iceberg.ManifestReader; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionStatisticsFile; import org.apache.iceberg.RewriteTablePathUtil; @@ -69,12 +74,14 @@ import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.JobGroupInfo; import org.apache.iceberg.spark.source.SerializableTableWithSize; import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; +import org.apache.spark.api.java.function.FlatMapFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -86,6 +93,7 @@ import org.apache.spark.sql.functions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import scala.Tuple2; public class RewriteTablePathSparkAction extends BaseSparkAction implements RewriteTablePath { @@ -309,23 +317,29 @@ private Result rebuildMetadata() { // rebuild manifest files Set metaFiles = rewriteManifestListResult.toRewrite(); + + // Enumerate the distinct position delete files referenced by the delete manifests being + // rewritten (metadata-only pass). + Set deleteFilesToRewrite = positionDeletesToRewrite(metaFiles); + + // Rewrite those delete files in parallel (deduped by path) and collect the actual size of each + // rewritten file. The size is measured from the writer on the executor that produced the file, + // avoiding both the end-of-job burst of getLength()/HEAD calls and the file system races where + // an in-progress write underreports its length. + Map rewrittenDeleteFileSizes = rewritePositionDeletes(deleteFilesToRewrite); + + // Rewrite manifests (metadata-only), stamping file_size_in_bytes from the measured sizes. RewriteContentFileResult rewriteManifestResult = - rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - - // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same - // position delete file can appear in multiple manifests. - int rewrittenDeleteFilesCount = - (int) - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)) - .size(); + rewriteManifests( + deltaSnapshots, + endMetadata, + metaFiles, + sparkContext().broadcast(rewrittenDeleteFileSizes)); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) + .rewrittenDeleteFilePathsCount(deleteFilesToRewrite.size()) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -562,7 +576,10 @@ public RewriteContentFileResult appendDeleteFile(RewriteResult r1) { /** Rewrite manifest files in a distributed manner and return rewritten data files path pairs. */ private RewriteContentFileResult rewriteManifests( - Set deltaSnapshots, TableMetadata tableMetadata, Set toRewrite) { + Set deltaSnapshots, + TableMetadata tableMetadata, + Set toRewrite, + Broadcast> rewrittenDeleteFileSizes) { if (toRewrite.isEmpty()) { return new RewriteContentFileResult(); } @@ -573,8 +590,6 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); - return manifestDS .repartition(toRewrite.size()) .map( @@ -585,7 +600,7 @@ private RewriteContentFileResult rewriteManifests( tableMetadata.formatVersion(), sourcePrefix, targetPrefix, - posDeleteReaderWriter), + rewrittenDeleteFileSizes), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -599,7 +614,7 @@ private static MapFunction toManifests( int format, String sourcePrefix, String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { + Broadcast> rewrittenDeleteFileSizes) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -625,7 +640,7 @@ private static MapFunction toManifests( format, sourcePrefix, targetPrefix, - posDeleteReaderWriter)); + rewrittenDeleteFileSizes)); break; default: throw new UnsupportedOperationException( @@ -672,7 +687,7 @@ private static RewriteResult writeDeleteManifest( int format, String sourcePrefix, String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { + Broadcast> rewrittenDeleteFileSizes) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -690,12 +705,121 @@ private static RewriteResult writeDeleteManifest( sourcePrefix, targetPrefix, stagingLocation, - posDeleteReaderWriter); + rewrittenDeleteFileSizes.value()); } catch (IOException e) { throw new RuntimeIOException(e); } } + /** + * Enumerate the distinct position delete files referenced by the delete manifests being + * rewritten. Equality delete files are excluded because they hold no absolute paths and are not + * rewritten. + */ + private Set positionDeletesToRewrite(Set metaFiles) { + Set deleteManifests = + metaFiles.stream() + .filter(manifest -> manifest.content() == ManifestContent.DELETES) + .collect(Collectors.toSet()); + if (deleteManifests.isEmpty()) { + return Collections.emptySet(); + } + + Encoder manifestFileEncoder = Encoders.javaSerialization(ManifestFile.class); + Dataset manifestDS = + spark().createDataset(Lists.newArrayList(deleteManifests), manifestFileEncoder); + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); + + List referencedDeleteFiles = + manifestDS + .repartition(deleteManifests.size()) + .flatMap(positionDeletesInManifest(tableBroadcast()), deleteFileEncoder) + .collectAsList(); + + // DeleteFile does not override equals(); DeleteFileSet dedupes by path so a delete file shared + // across manifests is rewritten only once. + DeleteFileSet distinct = DeleteFileSet.create(); + distinct.addAll(referencedDeleteFiles); + return distinct; + } + + private static FlatMapFunction positionDeletesInManifest( + Broadcast

tableArg) { + return manifestFile -> { + Table table = tableArg.getValue(); + List deleteFiles = Lists.newArrayList(); + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifestFile, table.io(), table.specs())) { + for (DeleteFile deleteFile : reader) { + if (deleteFile.content() == FileContent.POSITION_DELETES) { + deleteFiles.add(deleteFile.copy()); + } + } + } + return deleteFiles.iterator(); + }; + } + + /** + * Rewrite the given position delete files in parallel, returning a map from each source delete + * file path to the size of its rewritten file. + */ + private Map rewritePositionDeletes(Set toRewrite) { + if (toRewrite.isEmpty()) { + return Collections.emptyMap(); + } + + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); + Dataset deleteFileDS = + spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); + + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); + List> rewrittenSizes = + deleteFileDS + .repartition(toRewrite.size()) + .map( + rewritePositionDelete( + tableBroadcast(), + sourcePrefix, + targetPrefix, + stagingDir, + posDeleteReaderWriter), + Encoders.tuple(Encoders.STRING(), Encoders.LONG())) + .collectAsList(); + + Map sizesBySourcePath = Maps.newHashMap(); + for (Tuple2 entry : rewrittenSizes) { + sizesBySourcePath.put(entry._1(), entry._2()); + } + return sizesBySourcePath; + } + + private static MapFunction> rewritePositionDelete( + Broadcast
tableArg, + String sourcePrefixArg, + String targetPrefixArg, + String stagingLocationArg, + PositionDeleteReaderWriter posDeleteReaderWriter) { + return deleteFile -> { + FileIO io = tableArg.getValue().io(); + String newPath = + RewriteTablePathUtil.stagingPath( + deleteFile.location(), sourcePrefixArg, stagingLocationArg); + OutputFile outputFile = io.newOutputFile(newPath); + PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); + long rewrittenLength = + RewriteTablePathUtil.rewritePositionDeleteFileReturningLength( + deleteFile, + outputFile, + io, + spec, + sourcePrefixArg, + targetPrefixArg, + posDeleteReaderWriter); + return new Tuple2<>(deleteFile.location(), rewrittenLength); + }; + } + private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( diff --git a/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index 7fb1cd8e9019..fae1b0170eb7 100644 --- a/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -31,9 +32,13 @@ import org.apache.iceberg.ContentFile; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileContent; import org.apache.iceberg.FileFormat; import org.apache.iceberg.HasTableOperations; +import org.apache.iceberg.ManifestContent; import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestFiles; +import org.apache.iceberg.ManifestReader; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionStatisticsFile; import org.apache.iceberg.RewriteTablePathUtil; @@ -69,12 +74,14 @@ import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.JobGroupInfo; import org.apache.iceberg.spark.source.SerializableTableWithSize; import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; +import org.apache.spark.api.java.function.FlatMapFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -86,6 +93,7 @@ import org.apache.spark.sql.functions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import scala.Tuple2; public class RewriteTablePathSparkAction extends BaseSparkAction implements RewriteTablePath { @@ -309,23 +317,29 @@ private Result rebuildMetadata() { // rebuild manifest files Set metaFiles = rewriteManifestListResult.toRewrite(); + + // Enumerate the distinct position delete files referenced by the delete manifests being + // rewritten (metadata-only pass). + Set deleteFilesToRewrite = positionDeletesToRewrite(metaFiles); + + // Rewrite those delete files in parallel (deduped by path) and collect the actual size of each + // rewritten file. The size is measured from the writer on the executor that produced the file, + // avoiding both the end-of-job burst of getLength()/HEAD calls and the file system races where + // an in-progress write underreports its length. + Map rewrittenDeleteFileSizes = rewritePositionDeletes(deleteFilesToRewrite); + + // Rewrite manifests (metadata-only), stamping file_size_in_bytes from the measured sizes. RewriteContentFileResult rewriteManifestResult = - rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - - // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same - // position delete file can appear in multiple manifests. - int rewrittenDeleteFilesCount = - (int) - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)) - .size(); + rewriteManifests( + deltaSnapshots, + endMetadata, + metaFiles, + sparkContext().broadcast(rewrittenDeleteFileSizes)); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) + .rewrittenDeleteFilePathsCount(deleteFilesToRewrite.size()) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -562,7 +576,10 @@ public RewriteContentFileResult appendDeleteFile(RewriteResult r1) { /** Rewrite manifest files in a distributed manner and return rewritten data files path pairs. */ private RewriteContentFileResult rewriteManifests( - Set deltaSnapshots, TableMetadata tableMetadata, Set toRewrite) { + Set deltaSnapshots, + TableMetadata tableMetadata, + Set toRewrite, + Broadcast> rewrittenDeleteFileSizes) { if (toRewrite.isEmpty()) { return new RewriteContentFileResult(); } @@ -573,8 +590,6 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); - return manifestDS .repartition(toRewrite.size()) .map( @@ -585,7 +600,7 @@ private RewriteContentFileResult rewriteManifests( tableMetadata.formatVersion(), sourcePrefix, targetPrefix, - posDeleteReaderWriter), + rewrittenDeleteFileSizes), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -599,7 +614,7 @@ private static MapFunction toManifests( int format, String sourcePrefix, String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { + Broadcast> rewrittenDeleteFileSizes) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -625,7 +640,7 @@ private static MapFunction toManifests( format, sourcePrefix, targetPrefix, - posDeleteReaderWriter)); + rewrittenDeleteFileSizes)); break; default: throw new UnsupportedOperationException( @@ -672,7 +687,7 @@ private static RewriteResult writeDeleteManifest( int format, String sourcePrefix, String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { + Broadcast> rewrittenDeleteFileSizes) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -690,12 +705,121 @@ private static RewriteResult writeDeleteManifest( sourcePrefix, targetPrefix, stagingLocation, - posDeleteReaderWriter); + rewrittenDeleteFileSizes.value()); } catch (IOException e) { throw new RuntimeIOException(e); } } + /** + * Enumerate the distinct position delete files referenced by the delete manifests being + * rewritten. Equality delete files are excluded because they hold no absolute paths and are not + * rewritten. + */ + private Set positionDeletesToRewrite(Set metaFiles) { + Set deleteManifests = + metaFiles.stream() + .filter(manifest -> manifest.content() == ManifestContent.DELETES) + .collect(Collectors.toSet()); + if (deleteManifests.isEmpty()) { + return Collections.emptySet(); + } + + Encoder manifestFileEncoder = Encoders.javaSerialization(ManifestFile.class); + Dataset manifestDS = + spark().createDataset(Lists.newArrayList(deleteManifests), manifestFileEncoder); + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); + + List referencedDeleteFiles = + manifestDS + .repartition(deleteManifests.size()) + .flatMap(positionDeletesInManifest(tableBroadcast()), deleteFileEncoder) + .collectAsList(); + + // DeleteFile does not override equals(); DeleteFileSet dedupes by path so a delete file shared + // across manifests is rewritten only once. + DeleteFileSet distinct = DeleteFileSet.create(); + distinct.addAll(referencedDeleteFiles); + return distinct; + } + + private static FlatMapFunction positionDeletesInManifest( + Broadcast
tableArg) { + return manifestFile -> { + Table table = tableArg.getValue(); + List deleteFiles = Lists.newArrayList(); + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifestFile, table.io(), table.specs())) { + for (DeleteFile deleteFile : reader) { + if (deleteFile.content() == FileContent.POSITION_DELETES) { + deleteFiles.add(deleteFile.copy()); + } + } + } + return deleteFiles.iterator(); + }; + } + + /** + * Rewrite the given position delete files in parallel, returning a map from each source delete + * file path to the size of its rewritten file. + */ + private Map rewritePositionDeletes(Set toRewrite) { + if (toRewrite.isEmpty()) { + return Collections.emptyMap(); + } + + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); + Dataset deleteFileDS = + spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); + + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); + List> rewrittenSizes = + deleteFileDS + .repartition(toRewrite.size()) + .map( + rewritePositionDelete( + tableBroadcast(), + sourcePrefix, + targetPrefix, + stagingDir, + posDeleteReaderWriter), + Encoders.tuple(Encoders.STRING(), Encoders.LONG())) + .collectAsList(); + + Map sizesBySourcePath = Maps.newHashMap(); + for (Tuple2 entry : rewrittenSizes) { + sizesBySourcePath.put(entry._1(), entry._2()); + } + return sizesBySourcePath; + } + + private static MapFunction> rewritePositionDelete( + Broadcast
tableArg, + String sourcePrefixArg, + String targetPrefixArg, + String stagingLocationArg, + PositionDeleteReaderWriter posDeleteReaderWriter) { + return deleteFile -> { + FileIO io = tableArg.getValue().io(); + String newPath = + RewriteTablePathUtil.stagingPath( + deleteFile.location(), sourcePrefixArg, stagingLocationArg); + OutputFile outputFile = io.newOutputFile(newPath); + PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); + long rewrittenLength = + RewriteTablePathUtil.rewritePositionDeleteFileReturningLength( + deleteFile, + outputFile, + io, + spec, + sourcePrefixArg, + targetPrefixArg, + posDeleteReaderWriter); + return new Tuple2<>(deleteFile.location(), rewrittenLength); + }; + } + private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( diff --git a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index f18e776a56b1..fae1b0170eb7 100644 --- a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.io.OutputStreamWriter; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -31,9 +32,13 @@ import org.apache.iceberg.ContentFile; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileContent; import org.apache.iceberg.FileFormat; import org.apache.iceberg.HasTableOperations; +import org.apache.iceberg.ManifestContent; import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestFiles; +import org.apache.iceberg.ManifestReader; import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.PartitionStatisticsFile; import org.apache.iceberg.RewriteTablePathUtil; @@ -69,12 +74,14 @@ import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.JobGroupInfo; import org.apache.iceberg.spark.source.SerializableTableWithSize; import org.apache.iceberg.util.DeleteFileSet; import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; +import org.apache.spark.api.java.function.FlatMapFunction; import org.apache.spark.api.java.function.MapFunction; import org.apache.spark.api.java.function.ReduceFunction; import org.apache.spark.broadcast.Broadcast; @@ -86,6 +93,7 @@ import org.apache.spark.sql.functions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import scala.Tuple2; public class RewriteTablePathSparkAction extends BaseSparkAction implements RewriteTablePath { @@ -309,23 +317,29 @@ private Result rebuildMetadata() { // rebuild manifest files Set metaFiles = rewriteManifestListResult.toRewrite(); + + // Enumerate the distinct position delete files referenced by the delete manifests being + // rewritten (metadata-only pass). + Set deleteFilesToRewrite = positionDeletesToRewrite(metaFiles); + + // Rewrite those delete files in parallel (deduped by path) and collect the actual size of each + // rewritten file. The size is measured from the writer on the executor that produced the file, + // avoiding both the end-of-job burst of getLength()/HEAD calls and the file system races where + // an in-progress write underreports its length. + Map rewrittenDeleteFileSizes = rewritePositionDeletes(deleteFilesToRewrite); + + // Rewrite manifests (metadata-only), stamping file_size_in_bytes from the measured sizes. RewriteContentFileResult rewriteManifestResult = - rewriteManifests(deltaSnapshots, endMetadata, metaFiles); - - // DeleteFileSet deduplicates by path because DeleteFile lacks equals(), and the same - // position delete file can appear in multiple manifests. - int rewrittenDeleteFilesCount = - (int) - rewriteManifestResult.toRewrite().stream() - .filter(e -> e instanceof DeleteFile) - .map(e -> (DeleteFile) e) - .collect(Collectors.toCollection(DeleteFileSet::create)) - .size(); + rewriteManifests( + deltaSnapshots, + endMetadata, + metaFiles, + sparkContext().broadcast(rewrittenDeleteFileSizes)); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) - .rewrittenDeleteFilePathsCount(rewrittenDeleteFilesCount) + .rewrittenDeleteFilePathsCount(deleteFilesToRewrite.size()) .rewrittenManifestFilePathsCount(metaFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); @@ -562,7 +576,10 @@ public RewriteContentFileResult appendDeleteFile(RewriteResult r1) { /** Rewrite manifest files in a distributed manner and return rewritten data files path pairs. */ private RewriteContentFileResult rewriteManifests( - Set deltaSnapshots, TableMetadata tableMetadata, Set toRewrite) { + Set deltaSnapshots, + TableMetadata tableMetadata, + Set toRewrite, + Broadcast> rewrittenDeleteFileSizes) { if (toRewrite.isEmpty()) { return new RewriteContentFileResult(); } @@ -573,7 +590,6 @@ private RewriteContentFileResult rewriteManifests( Set deltaSnapshotIds = deltaSnapshots.stream().map(Snapshot::snapshotId).collect(Collectors.toSet()); - PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); return manifestDS .repartition(toRewrite.size()) .map( @@ -584,7 +600,7 @@ private RewriteContentFileResult rewriteManifests( tableMetadata.formatVersion(), sourcePrefix, targetPrefix, - posDeleteReaderWriter), + rewrittenDeleteFileSizes), Encoders.bean(RewriteContentFileResult.class)) // duplicates are expected here as the same data file can have different statuses // (e.g. added and deleted) @@ -598,7 +614,7 @@ private static MapFunction toManifests( int format, String sourcePrefix, String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { + Broadcast> rewrittenDeleteFileSizes) { return manifestFile -> { RewriteContentFileResult result = new RewriteContentFileResult(); @@ -624,7 +640,7 @@ private static MapFunction toManifests( format, sourcePrefix, targetPrefix, - posDeleteReaderWriter)); + rewrittenDeleteFileSizes)); break; default: throw new UnsupportedOperationException( @@ -671,7 +687,7 @@ private static RewriteResult writeDeleteManifest( int format, String sourcePrefix, String targetPrefix, - PositionDeleteReaderWriter posDeleteReaderWriter) { + Broadcast> rewrittenDeleteFileSizes) { try { String stagingPath = RewriteTablePathUtil.stagingPath(manifestFile.path(), sourcePrefix, stagingLocation); @@ -689,12 +705,121 @@ private static RewriteResult writeDeleteManifest( sourcePrefix, targetPrefix, stagingLocation, - posDeleteReaderWriter); + rewrittenDeleteFileSizes.value()); } catch (IOException e) { throw new RuntimeIOException(e); } } + /** + * Enumerate the distinct position delete files referenced by the delete manifests being + * rewritten. Equality delete files are excluded because they hold no absolute paths and are not + * rewritten. + */ + private Set positionDeletesToRewrite(Set metaFiles) { + Set deleteManifests = + metaFiles.stream() + .filter(manifest -> manifest.content() == ManifestContent.DELETES) + .collect(Collectors.toSet()); + if (deleteManifests.isEmpty()) { + return Collections.emptySet(); + } + + Encoder manifestFileEncoder = Encoders.javaSerialization(ManifestFile.class); + Dataset manifestDS = + spark().createDataset(Lists.newArrayList(deleteManifests), manifestFileEncoder); + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); + + List referencedDeleteFiles = + manifestDS + .repartition(deleteManifests.size()) + .flatMap(positionDeletesInManifest(tableBroadcast()), deleteFileEncoder) + .collectAsList(); + + // DeleteFile does not override equals(); DeleteFileSet dedupes by path so a delete file shared + // across manifests is rewritten only once. + DeleteFileSet distinct = DeleteFileSet.create(); + distinct.addAll(referencedDeleteFiles); + return distinct; + } + + private static FlatMapFunction positionDeletesInManifest( + Broadcast
tableArg) { + return manifestFile -> { + Table table = tableArg.getValue(); + List deleteFiles = Lists.newArrayList(); + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifestFile, table.io(), table.specs())) { + for (DeleteFile deleteFile : reader) { + if (deleteFile.content() == FileContent.POSITION_DELETES) { + deleteFiles.add(deleteFile.copy()); + } + } + } + return deleteFiles.iterator(); + }; + } + + /** + * Rewrite the given position delete files in parallel, returning a map from each source delete + * file path to the size of its rewritten file. + */ + private Map rewritePositionDeletes(Set toRewrite) { + if (toRewrite.isEmpty()) { + return Collections.emptyMap(); + } + + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); + Dataset deleteFileDS = + spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); + + PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); + List> rewrittenSizes = + deleteFileDS + .repartition(toRewrite.size()) + .map( + rewritePositionDelete( + tableBroadcast(), + sourcePrefix, + targetPrefix, + stagingDir, + posDeleteReaderWriter), + Encoders.tuple(Encoders.STRING(), Encoders.LONG())) + .collectAsList(); + + Map sizesBySourcePath = Maps.newHashMap(); + for (Tuple2 entry : rewrittenSizes) { + sizesBySourcePath.put(entry._1(), entry._2()); + } + return sizesBySourcePath; + } + + private static MapFunction> rewritePositionDelete( + Broadcast
tableArg, + String sourcePrefixArg, + String targetPrefixArg, + String stagingLocationArg, + PositionDeleteReaderWriter posDeleteReaderWriter) { + return deleteFile -> { + FileIO io = tableArg.getValue().io(); + String newPath = + RewriteTablePathUtil.stagingPath( + deleteFile.location(), sourcePrefixArg, stagingLocationArg); + OutputFile outputFile = io.newOutputFile(newPath); + PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); + long rewrittenLength = + RewriteTablePathUtil.rewritePositionDeleteFileReturningLength( + deleteFile, + outputFile, + io, + spec, + sourcePrefixArg, + targetPrefixArg, + posDeleteReaderWriter); + return new Tuple2<>(deleteFile.location(), rewrittenLength); + }; + } + private static class SparkPositionDeleteReaderWriter implements PositionDeleteReaderWriter { @Override public CloseableIterable reader( diff --git a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index 8f100232b45c..a1bd8d61bfd4 100644 --- a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -646,9 +646,9 @@ public void testPositionDeletesDeduplication() throws Exception { } // Regression test: when the same position delete file is referenced from manifests in different - // snapshots, each manifest is rewritten by a separate Spark task. Without per-task staging path - // isolation those tasks would collide on a shared path, either failing or recording an - // inconsistent file_size_in_bytes in one of the rewritten manifests. + // snapshots, it must be rewritten once and the resulting size stamped consistently into every + // manifest that references it. The delete file is enumerated and deduped by path before the + // rewrite, so its measured size is shared across all referencing delete manifests. @TestTemplate public void testSharedDeleteFileSizeAcrossManifests() throws Exception { assumeThat(formatVersion) @@ -713,6 +713,92 @@ public void testSharedDeleteFileSizeAcrossManifests() throws Exception { } } + // Regression test: a single delete manifest can reference multiple distinct position delete + // files, and each entry must be stamped with its own rewritten size. The two delete files carry + // a different number of records so they rewrite to different sizes, which catches a per-path size + // map that collapses entries to a single size or falls back to the stale original size. + @TestTemplate + public void testMultipleDistinctDeleteFileSizesAfterRewrite() throws Exception { + assumeThat(formatVersion) + .as("Format versions 3+ use DVs with different validation rules") + .isEqualTo(2); + + Table tableWithPosDeletes = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithDistinctDeletes"), + 2, + Map.of(TableProperties.DELETE_DEFAULT_FILE_FORMAT, "parquet")); + + List dataFiles = Lists.newArrayList(); + tableWithPosDeletes + .snapshots() + .forEach( + snapshot -> snapshot.addedDataFiles(tableWithPosDeletes.io()).forEach(dataFiles::add)); + assertThat(dataFiles).as("Expected two data files to reference from deletes").hasSize(2); + + // One delete file holds a single record, the other holds two, so they rewrite to distinct + // on-disk sizes. + File smallDeleteFile = + new File( + removePrefix( + tableWithPosDeletes.location() + "/data/deeply/nested/deletes-small.parquet")); + DeleteFile smallDelete = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(smallDeleteFile.toURI().toString()), + Lists.newArrayList(Pair.of(dataFiles.get(0).location(), 0L)), + formatVersion) + .first(); + + File largeDeleteFile = + new File( + removePrefix( + tableWithPosDeletes.location() + "/data/deeply/nested/deletes-large.parquet")); + DeleteFile largeDelete = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(largeDeleteFile.toURI().toString()), + Lists.newArrayList( + Pair.of(dataFiles.get(0).location(), 0L), + Pair.of(dataFiles.get(1).location(), 0L)), + formatVersion) + .first(); + + tableWithPosDeletes.newRowDelta().addDeletes(smallDelete).addDeletes(largeDelete).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithPosDeletes) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + List rewrittenSizes = Lists.newArrayList(); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + rewrittenSizes.add(manifestSize); + } + } + } + + assertThat(rewrittenSizes) + .as( + "The two distinct delete files should rewrite to distinct, independently recorded sizes") + .hasSize(2) + .doesNotHaveDuplicates(); + } + // Regression test: rewriting delete file paths changes the file size (since the // embedded data file paths may differ in length), but file_size_in_bytes in the rewritten // manifest was not updated. Readers that use file_size_in_bytes to elide a stat() call may From bbbe4666f794f143eda26fa55f0ef009047b20f3 Mon Sep 17 00:00:00 2001 From: Matt Butrovich Date: Mon, 22 Jun 2026 12:16:07 -0400 Subject: [PATCH 17/17] Core, Spark: dedupe DV rewrites by location and rename position-delete APIs per PR feedback --- .../apache/iceberg/RewriteTablePathUtil.java | 18 +- .../iceberg/TestRewriteTablePathUtil.java | 93 ++++++ .../actions/RewriteTablePathSparkAction.java | 64 ++-- .../actions/TestRewriteTablePathsAction.java | 304 +++++++++++++++++- .../actions/RewriteTablePathSparkAction.java | 64 ++-- .../actions/TestRewriteTablePathsAction.java | 304 +++++++++++++++++- .../actions/RewriteTablePathSparkAction.java | 64 ++-- .../actions/TestRewriteTablePathsAction.java | 89 +++++ 8 files changed, 878 insertions(+), 122 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java index c56c6cac908f..e72dbef6dbcc 100644 --- a/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java +++ b/core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java @@ -378,7 +378,7 @@ public static RewriteResult rewriteDataManifest( * @param stagingLocation staging location for rewritten files (referred delete file will be * rewritten here) * @return a copy plan of content files in the manifest that was rewritten - * @deprecated since 1.11.0, will be removed in 1.12.0; use the overload that accepts the map of + * @deprecated since 1.12.0, will be removed in 1.13.0; use the overload that accepts the map of * rewritten position delete file sizes. This overload records the original {@code * file_size_in_bytes}, which can be inconsistent with the rewritten file size on disk once * embedded data file paths change length. @@ -412,7 +412,7 @@ public static RewriteResult rewriteDeleteManifest( * Rewrite a delete manifest, replacing path references. * *

This is a metadata-only operation: position delete file content is rewritten separately (see - * {@link #rewritePositionDeleteFile}). The actual sizes of those rewritten files are supplied via + * {@link #rewritePositionDelete}). The actual sizes of those rewritten files are supplied via * {@code rewrittenDeleteFileSizes} and recorded in the manifest so that {@code * file_size_in_bytes} stays consistent with the rewritten file on disk. * @@ -506,9 +506,8 @@ private static RewriteResult writeDeleteFileEntry( switch (file.content()) { case POSITION_DELETES: - // Rewriting the embedded data file paths changes the file size, so record the actual size - // measured when the file was rewritten. Falls back to the original size for entries whose - // file was not rewritten (e.g. deleted entries that are not copied to the target). + // Path rewrites change the file size; use the measured size, falling back to the original + // for entries that were not rewritten (e.g. deleted entries not copied to the target). long fileSizeInBytes = rewrittenDeleteFileSizes.getOrDefault(file.location(), file.fileSizeInBytes()); DeleteFile posDeleteFile = @@ -672,7 +671,11 @@ PositionDeleteWriter writer( * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix to replace it * @param posDeleteReaderWriter class to read and write position delete files + * @deprecated since 1.12.0, will be removed in 1.13.0; use {@link #rewritePositionDelete} which + * returns the size of the rewritten file so callers can record an accurate {@code + * file_size_in_bytes}. */ + @Deprecated public static void rewritePositionDeleteFile( DeleteFile deleteFile, OutputFile outputFile, @@ -682,7 +685,7 @@ public static void rewritePositionDeleteFile( String targetPrefix, PositionDeleteReaderWriter posDeleteReaderWriter) throws IOException { - rewritePositionDeleteFileReturningLength( + rewritePositionDelete( deleteFile, outputFile, io, spec, sourcePrefix, targetPrefix, posDeleteReaderWriter); } @@ -704,7 +707,7 @@ public static void rewritePositionDeleteFile( * @param posDeleteReaderWriter class to read and write position delete files * @return the size in bytes of the rewritten file */ - public static long rewritePositionDeleteFileReturningLength( + public static long rewritePositionDelete( DeleteFile deleteFile, OutputFile outputFile, FileIO io, @@ -768,6 +771,7 @@ record = recordIt.next(); * @param io file io * @param sourcePrefix source prefix that will be replaced * @param targetPrefix target prefix to replace it + * @return the size in bytes of the rewritten DV file */ private static long rewriteDVFile( DeleteFile deleteFile, diff --git a/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java b/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java index e848865fb21f..1b0f5f6b1c70 100644 --- a/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java +++ b/core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java @@ -24,6 +24,8 @@ import java.io.IOException; import java.util.Set; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.io.OutputFile; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestTemplate; @@ -283,4 +285,95 @@ public void testRewritingMultiplePositionDeleteEntriesWithinManifestFile() throw assertThat(deleteFileRewriteResult.toRewrite()).hasSize(2); } + + // A position delete entry that is not rewritten (e.g. a DELETED entry not copied to the target) + // is absent from the measured-size map and must keep its original file_size_in_bytes. + @TestTemplate + public void testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries() + throws IOException { + assumeThat(formatVersion) + .as("Delete files only work for format version 2+") + .isGreaterThanOrEqualTo(2); + + String sourcePrefix = "/path/to/"; + String targetPrefix = "/path/new/"; + String stagingDir = "/staging/"; + + // FILE_A_DELETES is live, so its rewritten size is measured and supplied; FILE_B_DELETES is a + // DELETED entry, absent from the size map. + ManifestFile manifest = deleteManifestWithLiveAndDeletedEntry(FILE_A_DELETES, FILE_B_DELETES); + + long measuredSizeForA = 9999L; + OutputFile output = + Files.localOutput( + FileFormat.AVRO.addExtension( + temp.resolve("junit" + System.nanoTime()).toFile().toString())); + RewriteTablePathUtil.rewriteDeleteManifest( + manifest, + Set.of(1000L), + output, + table.io(), + formatVersion, + table.specs(), + sourcePrefix, + targetPrefix, + stagingDir, + ImmutableMap.of(FILE_A_DELETES.location(), measuredSizeForA)); + + InputFile rewrittenInput = output.toInputFile(); + ManifestFile rewritten = + new GenericManifestFile( + rewrittenInput.location(), + rewrittenInput.getLength(), + SPEC.specId(), + ManifestContent.DELETES, + 0L, + 0L, + 1000L, + null, + null, + null, + null, + null, + null, + null, + null, + null); + int seen = 0; + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(rewritten, table.io(), table.specs())) { + for (ManifestEntry entry : reader.entries()) { + seen++; + if (entry.status() == ManifestEntry.Status.DELETED) { + assertThat(entry.file().fileSizeInBytes()) + .as("DELETED entry should fall back to its original size") + .isEqualTo(FILE_B_DELETES.fileSizeInBytes()); + } else { + assertThat(entry.file().fileSizeInBytes()) + .as("Live entry should use the measured rewritten size") + .isEqualTo(measuredSizeForA); + } + } + } + + assertThat(seen).as("Both the live and deleted entries should be present").isEqualTo(2); + } + + private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, DeleteFile deleted) + throws IOException { + OutputFile manifestFile = + Files.localOutput( + FileFormat.AVRO.addExtension( + temp.resolve("junit" + System.nanoTime()).toFile().toString())); + ManifestWriter writer = + ManifestFiles.writeDeleteManifest(formatVersion, SPEC, manifestFile, 1000L); + try { + writer.add(live); + writer.delete(deleted, 1, null); + } finally { + writer.close(); + } + + return writer.toManifestFile(); + } } diff --git a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index fae1b0170eb7..11935e815e76 100644 --- a/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -279,7 +279,8 @@ private String jobDesc() { *

    *
  • Rebuild version files to staging *
  • Rebuild manifest list files to staging - *
  • Rebuild manifest to staging + *
  • Rewrite referenced position delete files to staging + *
  • Rebuild manifests to staging *
  • Get all files needed to move *
*/ @@ -315,32 +316,29 @@ private Result rebuildMetadata() { RewriteResult rewriteManifestListResult = new RewriteResult<>(); manifestListResults.forEach(rewriteManifestListResult::append); - // rebuild manifest files - Set metaFiles = rewriteManifestListResult.toRewrite(); - - // Enumerate the distinct position delete files referenced by the delete manifests being - // rewritten (metadata-only pass). - Set deleteFilesToRewrite = positionDeletesToRewrite(metaFiles); + Set manifestFiles = rewriteManifestListResult.toRewrite(); - // Rewrite those delete files in parallel (deduped by path) and collect the actual size of each - // rewritten file. The size is measured from the writer on the executor that produced the file, - // avoiding both the end-of-job burst of getLength()/HEAD calls and the file system races where - // an in-progress write underreports its length. + // rebuild position delete files + Set deleteManifests = + manifestFiles.stream() + .filter(manifest -> manifest.content() == ManifestContent.DELETES) + .collect(Collectors.toSet()); + Set deleteFilesToRewrite = positionDeletesToRewrite(deleteManifests); Map rewrittenDeleteFileSizes = rewritePositionDeletes(deleteFilesToRewrite); - // Rewrite manifests (metadata-only), stamping file_size_in_bytes from the measured sizes. + // rebuild manifest files RewriteContentFileResult rewriteManifestResult = rewriteManifests( deltaSnapshots, endMetadata, - metaFiles, + manifestFiles, sparkContext().broadcast(rewrittenDeleteFileSizes)); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) .rewrittenDeleteFilePathsCount(deleteFilesToRewrite.size()) - .rewrittenManifestFilePathsCount(metaFiles.size()) + .rewrittenManifestFilePathsCount(manifestFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); if (!createFileList) { @@ -712,15 +710,11 @@ private static RewriteResult writeDeleteManifest( } /** - * Enumerate the distinct position delete files referenced by the delete manifests being - * rewritten. Equality delete files are excluded because they hold no absolute paths and are not - * rewritten. + * Enumerate the distinct position delete files referenced by the given delete manifests. Deduped + * by identity (location, offset, size) so a file shared across manifests is counted once; the + * physical rewrite is further deduped by location in {@link #rewritePositionDeletes}. */ - private Set positionDeletesToRewrite(Set metaFiles) { - Set deleteManifests = - metaFiles.stream() - .filter(manifest -> manifest.content() == ManifestContent.DELETES) - .collect(Collectors.toSet()); + private Set positionDeletesToRewrite(Set deleteManifests) { if (deleteManifests.isEmpty()) { return Collections.emptySet(); } @@ -736,11 +730,7 @@ private Set positionDeletesToRewrite(Set metaFiles) { .flatMap(positionDeletesInManifest(tableBroadcast()), deleteFileEncoder) .collectAsList(); - // DeleteFile does not override equals(); DeleteFileSet dedupes by path so a delete file shared - // across manifests is rewritten only once. - DeleteFileSet distinct = DeleteFileSet.create(); - distinct.addAll(referencedDeleteFiles); - return distinct; + return DeleteFileSet.of(referencedDeleteFiles); } private static FlatMapFunction positionDeletesInManifest( @@ -762,21 +752,29 @@ private static FlatMapFunction positionDeletesInManife /** * Rewrite the given position delete files in parallel, returning a map from each source delete - * file path to the size of its rewritten file. + * file path to the size of its rewritten file. Physical files are deduped by location, so a + * Puffin file holding multiple DVs is rewritten once and its size keyed once. */ private Map rewritePositionDeletes(Set toRewrite) { if (toRewrite.isEmpty()) { return Collections.emptyMap(); } + // Multiple DVs can share one Puffin file at different blob offsets; rewrite each physical file + // once. The measured size is keyed by location and applied to every referencing manifest entry. + Map byLocation = Maps.newHashMapWithExpectedSize(toRewrite.size()); + for (DeleteFile deleteFile : toRewrite) { + byLocation.putIfAbsent(deleteFile.location(), deleteFile); + } + List physicalFiles = Lists.newArrayList(byLocation.values()); + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDS = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); + Dataset deleteFileDS = spark().createDataset(physicalFiles, deleteFileEncoder); PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); List> rewrittenSizes = deleteFileDS - .repartition(toRewrite.size()) + .repartition(physicalFiles.size()) .map( rewritePositionDelete( tableBroadcast(), @@ -787,7 +785,7 @@ private Map rewritePositionDeletes(Set toRewrite) { Encoders.tuple(Encoders.STRING(), Encoders.LONG())) .collectAsList(); - Map sizesBySourcePath = Maps.newHashMap(); + Map sizesBySourcePath = Maps.newHashMapWithExpectedSize(rewrittenSizes.size()); for (Tuple2 entry : rewrittenSizes) { sizesBySourcePath.put(entry._1(), entry._2()); } @@ -808,7 +806,7 @@ private static MapFunction> rewritePositionDele OutputFile outputFile = io.newOutputFile(newPath); PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); long rewrittenLength = - RewriteTablePathUtil.rewritePositionDeleteFileReturningLength( + RewriteTablePathUtil.rewritePositionDelete( deleteFile, outputFile, io, diff --git a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index c5db04762f21..9660beae187e 100644 --- a/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -30,6 +30,7 @@ import java.nio.file.Path; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.function.Predicate; @@ -41,15 +42,21 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.HasTableOperations; +import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestFiles; +import org.apache.iceberg.ManifestReader; import org.apache.iceberg.Parameter; import org.apache.iceberg.ParameterizedTestExtension; import org.apache.iceberg.Parameters; import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RowDelta; import org.apache.iceberg.Schema; import org.apache.iceberg.Snapshot; import org.apache.iceberg.SnapshotChanges; import org.apache.iceberg.StaticTableOperations; +import org.apache.iceberg.StructLike; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableProperties; @@ -62,14 +69,18 @@ import org.apache.iceberg.data.FileHelpers; import org.apache.iceberg.data.GenericRecord; import org.apache.iceberg.data.Record; +import org.apache.iceberg.deletes.BaseDVFileWriter; +import org.apache.iceberg.deletes.DVFileWriter; import org.apache.iceberg.deletes.PositionDelete; import org.apache.iceberg.hadoop.HadoopTables; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.io.OutputFileFactory; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Iterables; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.SparkCatalog; import org.apache.iceberg.spark.TestBase; import org.apache.iceberg.spark.source.ThreeColumnRecord; @@ -629,12 +640,7 @@ public void testPositionDeletesDeduplication() throws Exception { // in a new manifest, which will cause duplicate DeleteFile objects when processing tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); - // This should NOT throw AlreadyExistsException - the fix uses DeleteFileSet to deduplicate - // Without the fix (using Collectors.toSet()), this would fail because: - // 1. Both manifests contain entries for the same delete file - // 2. Processing returns two different DeleteFile objects for the same file - // 3. HashSet doesn't deduplicate them (DeleteFile doesn't override equals()) - // 4. rewritePositionDeletes tries to write the same file twice -> AlreadyExistsException + // This should NOT throw AlreadyExistsException RewriteTablePath.Result result = actions() .rewriteTablePath(tableWithPosDeletes) @@ -642,13 +648,295 @@ public void testPositionDeletesDeduplication() throws Exception { .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) .execute(); - // Verify the rewrite completed successfully - should have rewritten exactly 1 delete file - // (the duplicate should be deduplicated by DeleteFileSet) assertThat(result.rewrittenDeleteFilePathsCount()) .as("Should have rewritten exactly 1 delete file after deduplication") .isEqualTo(1); } + // Regression test: when the same position delete file is referenced from manifests in different + // snapshots, it must be rewritten once and the resulting size stamped consistently into every + // manifest that references it. The delete file is enumerated and deduped by path before the + // rewrite, so its measured size is shared across all referencing delete manifests. + @TestTemplate + public void testSharedDeleteFileSizeAcrossManifests() throws Exception { + assumeThat(formatVersion) + .as("Format versions 3+ use DVs with different validation rules") + .isEqualTo(2); + + Table tableWithPosDeletes = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithSharedDelete"), + 1, + Map.of(TableProperties.DELETE_DEFAULT_FILE_FORMAT, "parquet")); + + DataFile dataFile = + tableWithPosDeletes + .currentSnapshot() + .addedDataFiles(tableWithPosDeletes.io()) + .iterator() + .next(); + + List> deletes = Lists.newArrayList(Pair.of(dataFile.location(), 0L)); + File deleteFile = + new File( + removePrefix(tableWithPosDeletes.location() + "/data/deeply/nested/deletes.parquet")); + DeleteFile positionDeletes = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(deleteFile.toURI().toString()), + deletes, + formatVersion) + .first(); + + tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); + tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithPosDeletes) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + List deleteManifests = + targetTable.currentSnapshot().deleteManifests(targetTable.io()); + assertThat(deleteManifests) + .as("Expected the shared delete file to be referenced by multiple manifests") + .hasSizeGreaterThanOrEqualTo(2); + for (ManifestFile manifest : deleteManifests) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + } + } + } + } + + // Regression test: a single delete manifest can reference multiple distinct position delete + // files, and each entry must be stamped with its own rewritten size. The two delete files carry + // a different number of records so they rewrite to different sizes, which catches a per-path size + // map that collapses entries to a single size or falls back to the stale original size. + @TestTemplate + public void testMultipleDistinctDeleteFileSizesAfterRewrite() throws Exception { + assumeThat(formatVersion) + .as("Format versions 3+ use DVs with different validation rules") + .isEqualTo(2); + + Table tableWithPosDeletes = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithDistinctDeletes"), + 2, + Map.of(TableProperties.DELETE_DEFAULT_FILE_FORMAT, "parquet")); + + List dataFiles = Lists.newArrayList(); + tableWithPosDeletes + .snapshots() + .forEach( + snapshot -> snapshot.addedDataFiles(tableWithPosDeletes.io()).forEach(dataFiles::add)); + assertThat(dataFiles).as("Expected two data files to reference from deletes").hasSize(2); + + // One delete file holds a single record, the other holds two, so they rewrite to distinct + // on-disk sizes. + File smallDeleteFile = + new File( + removePrefix( + tableWithPosDeletes.location() + "/data/deeply/nested/deletes-small.parquet")); + DeleteFile smallDelete = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(smallDeleteFile.toURI().toString()), + Lists.newArrayList(Pair.of(dataFiles.get(0).location(), 0L)), + formatVersion) + .first(); + + File largeDeleteFile = + new File( + removePrefix( + tableWithPosDeletes.location() + "/data/deeply/nested/deletes-large.parquet")); + DeleteFile largeDelete = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(largeDeleteFile.toURI().toString()), + Lists.newArrayList( + Pair.of(dataFiles.get(0).location(), 0L), + Pair.of(dataFiles.get(1).location(), 0L)), + formatVersion) + .first(); + + tableWithPosDeletes.newRowDelta().addDeletes(smallDelete).addDeletes(largeDelete).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithPosDeletes) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + List rewrittenSizes = Lists.newArrayList(); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + rewrittenSizes.add(manifestSize); + } + } + } + + assertThat(rewrittenSizes) + .as( + "The two distinct delete files should rewrite to distinct, independently recorded sizes") + .hasSize(2) + .doesNotHaveDuplicates(); + } + + // Regression test: rewriting delete file paths changes the file size (since the + // embedded data file paths may differ in length), but file_size_in_bytes in the rewritten + // manifest was not updated. Readers that use file_size_in_bytes to elide a stat() call may + // fail. + @TestTemplate + public void testDeleteFileSizeInBytesAfterRewrite() throws Exception { + List> deletes = + Lists.newArrayList( + Pair.of( + SnapshotChanges.builderFor(table) + .build() + .addedDataFiles() + .iterator() + .next() + .location(), + 0L)); + + File file = new File(removePrefix(table.location() + "/data/deeply/nested/deletes.parquet")); + DeleteFile positionDeletes = + FileHelpers.writeDeleteFile( + table, table.io().newOutputFile(file.toURI().toString()), deletes, formatVersion) + .first(); + table.newRowDelta().addDeletes(positionDeletes).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(table) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(table.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + } + } + } + } + + // Regression test: a single Puffin file can hold multiple DVs (one blob per data file) + // referenced by distinct DeleteFile entries that share the same location. The rewrite must + // rewrite + // the physical Puffin file once (rather than colliding on the staging path) and stamp the + // rewritten size into every referencing manifest entry. + @TestTemplate + public void testSharedPuffinDeleteFileSizeAfterRewrite() throws Exception { + assumeThat(formatVersion) + .as("DVs are introduced in v3; v4 writes parquet manifests the test setup cannot read") + .isEqualTo(3); + + Table tableWithDVs = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithSharedPuffin"), 2); + + List dataFilePaths = Lists.newArrayList(); + tableWithDVs + .snapshots() + .forEach( + snapshot -> + snapshot + .addedDataFiles(tableWithDVs.io()) + .forEach(dataFile -> dataFilePaths.add(dataFile.location()))); + assertThat(dataFilePaths).as("Expected two data files to back two DVs").hasSize(2); + + List dvs = writeDVsForDataFiles(tableWithDVs, dataFilePaths); + assertThat(dvs) + .as("Both DVs should live in a single Puffin file") + .hasSize(2) + .allSatisfy(dv -> assertThat(dv.location()).isEqualTo(dvs.get(0).location())); + + RowDelta rowDelta = tableWithDVs.newRowDelta(); + dvs.forEach(rowDelta::addDeletes); + rowDelta.commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithDVs) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithDVs.location(), targetTableLocation()) + .execute(); + assertThat(result.rewrittenDeleteFilePathsCount()) + .as("Two DVs are two delete files with rewritten paths") + .isEqualTo(2); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + Set rewrittenLocations = Sets.newHashSet(); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + rewrittenLocations.add(df.location()); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(df.fileSizeInBytes()) + .as("file_size_in_bytes should match the rewritten Puffin size for %s", df.location()) + .isEqualTo(actualSize); + } + } + } + assertThat(rewrittenLocations) + .as("Both DVs should point at the single rewritten Puffin file") + .hasSize(1); + } + + // Writes one DV per data file path into a single Puffin file, returning the resulting DeleteFiles + // (which share a location but carry distinct blob offsets). + private List writeDVsForDataFiles(Table targetTable, List dataFilePaths) + throws IOException { + OutputFileFactory fileFactory = + OutputFileFactory.builderFor(targetTable, 1, 1).format(FileFormat.PUFFIN).build(); + DVFileWriter writer = new BaseDVFileWriter(fileFactory, p -> null); + try (DVFileWriter closeableWriter = writer) { + for (String path : dataFilePaths) { + closeableWriter.delete(path, 0L, targetTable.spec(), (StructLike) null); + } + } + + return writer.result().deleteFiles(); + } + @TestTemplate public void testEqualityDeletes() throws Exception { Table sourceTable = createTableWithSnapshots(newTableLocation(), 1); diff --git a/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index fae1b0170eb7..11935e815e76 100644 --- a/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -279,7 +279,8 @@ private String jobDesc() { *
    *
  • Rebuild version files to staging *
  • Rebuild manifest list files to staging - *
  • Rebuild manifest to staging + *
  • Rewrite referenced position delete files to staging + *
  • Rebuild manifests to staging *
  • Get all files needed to move *
*/ @@ -315,32 +316,29 @@ private Result rebuildMetadata() { RewriteResult rewriteManifestListResult = new RewriteResult<>(); manifestListResults.forEach(rewriteManifestListResult::append); - // rebuild manifest files - Set metaFiles = rewriteManifestListResult.toRewrite(); - - // Enumerate the distinct position delete files referenced by the delete manifests being - // rewritten (metadata-only pass). - Set deleteFilesToRewrite = positionDeletesToRewrite(metaFiles); + Set manifestFiles = rewriteManifestListResult.toRewrite(); - // Rewrite those delete files in parallel (deduped by path) and collect the actual size of each - // rewritten file. The size is measured from the writer on the executor that produced the file, - // avoiding both the end-of-job burst of getLength()/HEAD calls and the file system races where - // an in-progress write underreports its length. + // rebuild position delete files + Set deleteManifests = + manifestFiles.stream() + .filter(manifest -> manifest.content() == ManifestContent.DELETES) + .collect(Collectors.toSet()); + Set deleteFilesToRewrite = positionDeletesToRewrite(deleteManifests); Map rewrittenDeleteFileSizes = rewritePositionDeletes(deleteFilesToRewrite); - // Rewrite manifests (metadata-only), stamping file_size_in_bytes from the measured sizes. + // rebuild manifest files RewriteContentFileResult rewriteManifestResult = rewriteManifests( deltaSnapshots, endMetadata, - metaFiles, + manifestFiles, sparkContext().broadcast(rewrittenDeleteFileSizes)); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) .rewrittenDeleteFilePathsCount(deleteFilesToRewrite.size()) - .rewrittenManifestFilePathsCount(metaFiles.size()) + .rewrittenManifestFilePathsCount(manifestFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); if (!createFileList) { @@ -712,15 +710,11 @@ private static RewriteResult writeDeleteManifest( } /** - * Enumerate the distinct position delete files referenced by the delete manifests being - * rewritten. Equality delete files are excluded because they hold no absolute paths and are not - * rewritten. + * Enumerate the distinct position delete files referenced by the given delete manifests. Deduped + * by identity (location, offset, size) so a file shared across manifests is counted once; the + * physical rewrite is further deduped by location in {@link #rewritePositionDeletes}. */ - private Set positionDeletesToRewrite(Set metaFiles) { - Set deleteManifests = - metaFiles.stream() - .filter(manifest -> manifest.content() == ManifestContent.DELETES) - .collect(Collectors.toSet()); + private Set positionDeletesToRewrite(Set deleteManifests) { if (deleteManifests.isEmpty()) { return Collections.emptySet(); } @@ -736,11 +730,7 @@ private Set positionDeletesToRewrite(Set metaFiles) { .flatMap(positionDeletesInManifest(tableBroadcast()), deleteFileEncoder) .collectAsList(); - // DeleteFile does not override equals(); DeleteFileSet dedupes by path so a delete file shared - // across manifests is rewritten only once. - DeleteFileSet distinct = DeleteFileSet.create(); - distinct.addAll(referencedDeleteFiles); - return distinct; + return DeleteFileSet.of(referencedDeleteFiles); } private static FlatMapFunction positionDeletesInManifest( @@ -762,21 +752,29 @@ private static FlatMapFunction positionDeletesInManife /** * Rewrite the given position delete files in parallel, returning a map from each source delete - * file path to the size of its rewritten file. + * file path to the size of its rewritten file. Physical files are deduped by location, so a + * Puffin file holding multiple DVs is rewritten once and its size keyed once. */ private Map rewritePositionDeletes(Set toRewrite) { if (toRewrite.isEmpty()) { return Collections.emptyMap(); } + // Multiple DVs can share one Puffin file at different blob offsets; rewrite each physical file + // once. The measured size is keyed by location and applied to every referencing manifest entry. + Map byLocation = Maps.newHashMapWithExpectedSize(toRewrite.size()); + for (DeleteFile deleteFile : toRewrite) { + byLocation.putIfAbsent(deleteFile.location(), deleteFile); + } + List physicalFiles = Lists.newArrayList(byLocation.values()); + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDS = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); + Dataset deleteFileDS = spark().createDataset(physicalFiles, deleteFileEncoder); PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); List> rewrittenSizes = deleteFileDS - .repartition(toRewrite.size()) + .repartition(physicalFiles.size()) .map( rewritePositionDelete( tableBroadcast(), @@ -787,7 +785,7 @@ private Map rewritePositionDeletes(Set toRewrite) { Encoders.tuple(Encoders.STRING(), Encoders.LONG())) .collectAsList(); - Map sizesBySourcePath = Maps.newHashMap(); + Map sizesBySourcePath = Maps.newHashMapWithExpectedSize(rewrittenSizes.size()); for (Tuple2 entry : rewrittenSizes) { sizesBySourcePath.put(entry._1(), entry._2()); } @@ -808,7 +806,7 @@ private static MapFunction> rewritePositionDele OutputFile outputFile = io.newOutputFile(newPath); PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); long rewrittenLength = - RewriteTablePathUtil.rewritePositionDeleteFileReturningLength( + RewriteTablePathUtil.rewritePositionDelete( deleteFile, outputFile, io, diff --git a/spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index c5db04762f21..9660beae187e 100644 --- a/spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -30,6 +30,7 @@ import java.nio.file.Path; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.function.Predicate; @@ -41,15 +42,21 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.HasTableOperations; +import org.apache.iceberg.ManifestFile; +import org.apache.iceberg.ManifestFiles; +import org.apache.iceberg.ManifestReader; import org.apache.iceberg.Parameter; import org.apache.iceberg.ParameterizedTestExtension; import org.apache.iceberg.Parameters; import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RowDelta; import org.apache.iceberg.Schema; import org.apache.iceberg.Snapshot; import org.apache.iceberg.SnapshotChanges; import org.apache.iceberg.StaticTableOperations; +import org.apache.iceberg.StructLike; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableProperties; @@ -62,14 +69,18 @@ import org.apache.iceberg.data.FileHelpers; import org.apache.iceberg.data.GenericRecord; import org.apache.iceberg.data.Record; +import org.apache.iceberg.deletes.BaseDVFileWriter; +import org.apache.iceberg.deletes.DVFileWriter; import org.apache.iceberg.deletes.PositionDelete; import org.apache.iceberg.hadoop.HadoopTables; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.io.OutputFileFactory; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Iterables; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.SparkCatalog; import org.apache.iceberg.spark.TestBase; import org.apache.iceberg.spark.source.ThreeColumnRecord; @@ -629,12 +640,7 @@ public void testPositionDeletesDeduplication() throws Exception { // in a new manifest, which will cause duplicate DeleteFile objects when processing tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); - // This should NOT throw AlreadyExistsException - the fix uses DeleteFileSet to deduplicate - // Without the fix (using Collectors.toSet()), this would fail because: - // 1. Both manifests contain entries for the same delete file - // 2. Processing returns two different DeleteFile objects for the same file - // 3. HashSet doesn't deduplicate them (DeleteFile doesn't override equals()) - // 4. rewritePositionDeletes tries to write the same file twice -> AlreadyExistsException + // This should NOT throw AlreadyExistsException RewriteTablePath.Result result = actions() .rewriteTablePath(tableWithPosDeletes) @@ -642,13 +648,295 @@ public void testPositionDeletesDeduplication() throws Exception { .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) .execute(); - // Verify the rewrite completed successfully - should have rewritten exactly 1 delete file - // (the duplicate should be deduplicated by DeleteFileSet) assertThat(result.rewrittenDeleteFilePathsCount()) .as("Should have rewritten exactly 1 delete file after deduplication") .isEqualTo(1); } + // Regression test: when the same position delete file is referenced from manifests in different + // snapshots, it must be rewritten once and the resulting size stamped consistently into every + // manifest that references it. The delete file is enumerated and deduped by path before the + // rewrite, so its measured size is shared across all referencing delete manifests. + @TestTemplate + public void testSharedDeleteFileSizeAcrossManifests() throws Exception { + assumeThat(formatVersion) + .as("Format versions 3+ use DVs with different validation rules") + .isEqualTo(2); + + Table tableWithPosDeletes = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithSharedDelete"), + 1, + Map.of(TableProperties.DELETE_DEFAULT_FILE_FORMAT, "parquet")); + + DataFile dataFile = + tableWithPosDeletes + .currentSnapshot() + .addedDataFiles(tableWithPosDeletes.io()) + .iterator() + .next(); + + List> deletes = Lists.newArrayList(Pair.of(dataFile.location(), 0L)); + File deleteFile = + new File( + removePrefix(tableWithPosDeletes.location() + "/data/deeply/nested/deletes.parquet")); + DeleteFile positionDeletes = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(deleteFile.toURI().toString()), + deletes, + formatVersion) + .first(); + + tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); + tableWithPosDeletes.newRowDelta().addDeletes(positionDeletes).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithPosDeletes) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + List deleteManifests = + targetTable.currentSnapshot().deleteManifests(targetTable.io()); + assertThat(deleteManifests) + .as("Expected the shared delete file to be referenced by multiple manifests") + .hasSizeGreaterThanOrEqualTo(2); + for (ManifestFile manifest : deleteManifests) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + } + } + } + } + + // Regression test: a single delete manifest can reference multiple distinct position delete + // files, and each entry must be stamped with its own rewritten size. The two delete files carry + // a different number of records so they rewrite to different sizes, which catches a per-path size + // map that collapses entries to a single size or falls back to the stale original size. + @TestTemplate + public void testMultipleDistinctDeleteFileSizesAfterRewrite() throws Exception { + assumeThat(formatVersion) + .as("Format versions 3+ use DVs with different validation rules") + .isEqualTo(2); + + Table tableWithPosDeletes = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithDistinctDeletes"), + 2, + Map.of(TableProperties.DELETE_DEFAULT_FILE_FORMAT, "parquet")); + + List dataFiles = Lists.newArrayList(); + tableWithPosDeletes + .snapshots() + .forEach( + snapshot -> snapshot.addedDataFiles(tableWithPosDeletes.io()).forEach(dataFiles::add)); + assertThat(dataFiles).as("Expected two data files to reference from deletes").hasSize(2); + + // One delete file holds a single record, the other holds two, so they rewrite to distinct + // on-disk sizes. + File smallDeleteFile = + new File( + removePrefix( + tableWithPosDeletes.location() + "/data/deeply/nested/deletes-small.parquet")); + DeleteFile smallDelete = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(smallDeleteFile.toURI().toString()), + Lists.newArrayList(Pair.of(dataFiles.get(0).location(), 0L)), + formatVersion) + .first(); + + File largeDeleteFile = + new File( + removePrefix( + tableWithPosDeletes.location() + "/data/deeply/nested/deletes-large.parquet")); + DeleteFile largeDelete = + FileHelpers.writeDeleteFile( + tableWithPosDeletes, + tableWithPosDeletes.io().newOutputFile(largeDeleteFile.toURI().toString()), + Lists.newArrayList( + Pair.of(dataFiles.get(0).location(), 0L), + Pair.of(dataFiles.get(1).location(), 0L)), + formatVersion) + .first(); + + tableWithPosDeletes.newRowDelta().addDeletes(smallDelete).addDeletes(largeDelete).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithPosDeletes) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithPosDeletes.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + List rewrittenSizes = Lists.newArrayList(); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + rewrittenSizes.add(manifestSize); + } + } + } + + assertThat(rewrittenSizes) + .as( + "The two distinct delete files should rewrite to distinct, independently recorded sizes") + .hasSize(2) + .doesNotHaveDuplicates(); + } + + // Regression test: rewriting delete file paths changes the file size (since the + // embedded data file paths may differ in length), but file_size_in_bytes in the rewritten + // manifest was not updated. Readers that use file_size_in_bytes to elide a stat() call may + // fail. + @TestTemplate + public void testDeleteFileSizeInBytesAfterRewrite() throws Exception { + List> deletes = + Lists.newArrayList( + Pair.of( + SnapshotChanges.builderFor(table) + .build() + .addedDataFiles() + .iterator() + .next() + .location(), + 0L)); + + File file = new File(removePrefix(table.location() + "/data/deeply/nested/deletes.parquet")); + DeleteFile positionDeletes = + FileHelpers.writeDeleteFile( + table, table.io().newOutputFile(file.toURI().toString()), deletes, formatVersion) + .first(); + table.newRowDelta().addDeletes(positionDeletes).commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(table) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(table.location(), targetTableLocation()) + .execute(); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + long manifestSize = df.fileSizeInBytes(); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(manifestSize) + .as( + "file_size_in_bytes in rewritten manifest should match actual file size for %s", + df.location()) + .isEqualTo(actualSize); + } + } + } + } + + // Regression test: a single Puffin file can hold multiple DVs (one blob per data file) + // referenced by distinct DeleteFile entries that share the same location. The rewrite must + // rewrite + // the physical Puffin file once (rather than colliding on the staging path) and stamp the + // rewritten size into every referencing manifest entry. + @TestTemplate + public void testSharedPuffinDeleteFileSizeAfterRewrite() throws Exception { + assumeThat(formatVersion) + .as("DVs are introduced in v3; v4 writes parquet manifests the test setup cannot read") + .isEqualTo(3); + + Table tableWithDVs = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithSharedPuffin"), 2); + + List dataFilePaths = Lists.newArrayList(); + tableWithDVs + .snapshots() + .forEach( + snapshot -> + snapshot + .addedDataFiles(tableWithDVs.io()) + .forEach(dataFile -> dataFilePaths.add(dataFile.location()))); + assertThat(dataFilePaths).as("Expected two data files to back two DVs").hasSize(2); + + List dvs = writeDVsForDataFiles(tableWithDVs, dataFilePaths); + assertThat(dvs) + .as("Both DVs should live in a single Puffin file") + .hasSize(2) + .allSatisfy(dv -> assertThat(dv.location()).isEqualTo(dvs.get(0).location())); + + RowDelta rowDelta = tableWithDVs.newRowDelta(); + dvs.forEach(rowDelta::addDeletes); + rowDelta.commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithDVs) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithDVs.location(), targetTableLocation()) + .execute(); + assertThat(result.rewrittenDeleteFilePathsCount()) + .as("Two DVs are two delete files with rewritten paths") + .isEqualTo(2); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + Set rewrittenLocations = Sets.newHashSet(); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + rewrittenLocations.add(df.location()); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(df.fileSizeInBytes()) + .as("file_size_in_bytes should match the rewritten Puffin size for %s", df.location()) + .isEqualTo(actualSize); + } + } + } + assertThat(rewrittenLocations) + .as("Both DVs should point at the single rewritten Puffin file") + .hasSize(1); + } + + // Writes one DV per data file path into a single Puffin file, returning the resulting DeleteFiles + // (which share a location but carry distinct blob offsets). + private List writeDVsForDataFiles(Table targetTable, List dataFilePaths) + throws IOException { + OutputFileFactory fileFactory = + OutputFileFactory.builderFor(targetTable, 1, 1).format(FileFormat.PUFFIN).build(); + DVFileWriter writer = new BaseDVFileWriter(fileFactory, p -> null); + try (DVFileWriter closeableWriter = writer) { + for (String path : dataFilePaths) { + closeableWriter.delete(path, 0L, targetTable.spec(), (StructLike) null); + } + } + + return writer.result().deleteFiles(); + } + @TestTemplate public void testEqualityDeletes() throws Exception { Table sourceTable = createTableWithSnapshots(newTableLocation(), 1); diff --git a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java index fae1b0170eb7..11935e815e76 100644 --- a/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java +++ b/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java @@ -279,7 +279,8 @@ private String jobDesc() { *
    *
  • Rebuild version files to staging *
  • Rebuild manifest list files to staging - *
  • Rebuild manifest to staging + *
  • Rewrite referenced position delete files to staging + *
  • Rebuild manifests to staging *
  • Get all files needed to move *
*/ @@ -315,32 +316,29 @@ private Result rebuildMetadata() { RewriteResult rewriteManifestListResult = new RewriteResult<>(); manifestListResults.forEach(rewriteManifestListResult::append); - // rebuild manifest files - Set metaFiles = rewriteManifestListResult.toRewrite(); - - // Enumerate the distinct position delete files referenced by the delete manifests being - // rewritten (metadata-only pass). - Set deleteFilesToRewrite = positionDeletesToRewrite(metaFiles); + Set manifestFiles = rewriteManifestListResult.toRewrite(); - // Rewrite those delete files in parallel (deduped by path) and collect the actual size of each - // rewritten file. The size is measured from the writer on the executor that produced the file, - // avoiding both the end-of-job burst of getLength()/HEAD calls and the file system races where - // an in-progress write underreports its length. + // rebuild position delete files + Set deleteManifests = + manifestFiles.stream() + .filter(manifest -> manifest.content() == ManifestContent.DELETES) + .collect(Collectors.toSet()); + Set deleteFilesToRewrite = positionDeletesToRewrite(deleteManifests); Map rewrittenDeleteFileSizes = rewritePositionDeletes(deleteFilesToRewrite); - // Rewrite manifests (metadata-only), stamping file_size_in_bytes from the measured sizes. + // rebuild manifest files RewriteContentFileResult rewriteManifestResult = rewriteManifests( deltaSnapshots, endMetadata, - metaFiles, + manifestFiles, sparkContext().broadcast(rewrittenDeleteFileSizes)); ImmutableRewriteTablePath.Result.Builder builder = ImmutableRewriteTablePath.Result.builder() .stagingLocation(stagingDir) .rewrittenDeleteFilePathsCount(deleteFilesToRewrite.size()) - .rewrittenManifestFilePathsCount(metaFiles.size()) + .rewrittenManifestFilePathsCount(manifestFiles.size()) .latestVersion(RewriteTablePathUtil.fileName(endVersionName)); if (!createFileList) { @@ -712,15 +710,11 @@ private static RewriteResult writeDeleteManifest( } /** - * Enumerate the distinct position delete files referenced by the delete manifests being - * rewritten. Equality delete files are excluded because they hold no absolute paths and are not - * rewritten. + * Enumerate the distinct position delete files referenced by the given delete manifests. Deduped + * by identity (location, offset, size) so a file shared across manifests is counted once; the + * physical rewrite is further deduped by location in {@link #rewritePositionDeletes}. */ - private Set positionDeletesToRewrite(Set metaFiles) { - Set deleteManifests = - metaFiles.stream() - .filter(manifest -> manifest.content() == ManifestContent.DELETES) - .collect(Collectors.toSet()); + private Set positionDeletesToRewrite(Set deleteManifests) { if (deleteManifests.isEmpty()) { return Collections.emptySet(); } @@ -736,11 +730,7 @@ private Set positionDeletesToRewrite(Set metaFiles) { .flatMap(positionDeletesInManifest(tableBroadcast()), deleteFileEncoder) .collectAsList(); - // DeleteFile does not override equals(); DeleteFileSet dedupes by path so a delete file shared - // across manifests is rewritten only once. - DeleteFileSet distinct = DeleteFileSet.create(); - distinct.addAll(referencedDeleteFiles); - return distinct; + return DeleteFileSet.of(referencedDeleteFiles); } private static FlatMapFunction positionDeletesInManifest( @@ -762,21 +752,29 @@ private static FlatMapFunction positionDeletesInManife /** * Rewrite the given position delete files in parallel, returning a map from each source delete - * file path to the size of its rewritten file. + * file path to the size of its rewritten file. Physical files are deduped by location, so a + * Puffin file holding multiple DVs is rewritten once and its size keyed once. */ private Map rewritePositionDeletes(Set toRewrite) { if (toRewrite.isEmpty()) { return Collections.emptyMap(); } + // Multiple DVs can share one Puffin file at different blob offsets; rewrite each physical file + // once. The measured size is keyed by location and applied to every referencing manifest entry. + Map byLocation = Maps.newHashMapWithExpectedSize(toRewrite.size()); + for (DeleteFile deleteFile : toRewrite) { + byLocation.putIfAbsent(deleteFile.location(), deleteFile); + } + List physicalFiles = Lists.newArrayList(byLocation.values()); + Encoder deleteFileEncoder = Encoders.javaSerialization(DeleteFile.class); - Dataset deleteFileDS = - spark().createDataset(Lists.newArrayList(toRewrite), deleteFileEncoder); + Dataset deleteFileDS = spark().createDataset(physicalFiles, deleteFileEncoder); PositionDeleteReaderWriter posDeleteReaderWriter = new SparkPositionDeleteReaderWriter(); List> rewrittenSizes = deleteFileDS - .repartition(toRewrite.size()) + .repartition(physicalFiles.size()) .map( rewritePositionDelete( tableBroadcast(), @@ -787,7 +785,7 @@ private Map rewritePositionDeletes(Set toRewrite) { Encoders.tuple(Encoders.STRING(), Encoders.LONG())) .collectAsList(); - Map sizesBySourcePath = Maps.newHashMap(); + Map sizesBySourcePath = Maps.newHashMapWithExpectedSize(rewrittenSizes.size()); for (Tuple2 entry : rewrittenSizes) { sizesBySourcePath.put(entry._1(), entry._2()); } @@ -808,7 +806,7 @@ private static MapFunction> rewritePositionDele OutputFile outputFile = io.newOutputFile(newPath); PartitionSpec spec = tableArg.getValue().specs().get(deleteFile.specId()); long rewrittenLength = - RewriteTablePathUtil.rewritePositionDeleteFileReturningLength( + RewriteTablePathUtil.rewritePositionDelete( deleteFile, outputFile, io, diff --git a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java index a1bd8d61bfd4..9660beae187e 100644 --- a/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java +++ b/spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java @@ -30,6 +30,7 @@ import java.nio.file.Path; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.function.Predicate; @@ -41,6 +42,7 @@ import org.apache.iceberg.BaseTable; import org.apache.iceberg.DataFile; import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.HasTableOperations; import org.apache.iceberg.ManifestFile; import org.apache.iceberg.ManifestFiles; @@ -49,10 +51,12 @@ import org.apache.iceberg.ParameterizedTestExtension; import org.apache.iceberg.Parameters; import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RowDelta; import org.apache.iceberg.Schema; import org.apache.iceberg.Snapshot; import org.apache.iceberg.SnapshotChanges; import org.apache.iceberg.StaticTableOperations; +import org.apache.iceberg.StructLike; import org.apache.iceberg.Table; import org.apache.iceberg.TableMetadata; import org.apache.iceberg.TableProperties; @@ -65,14 +69,18 @@ import org.apache.iceberg.data.FileHelpers; import org.apache.iceberg.data.GenericRecord; import org.apache.iceberg.data.Record; +import org.apache.iceberg.deletes.BaseDVFileWriter; +import org.apache.iceberg.deletes.DVFileWriter; import org.apache.iceberg.deletes.PositionDelete; import org.apache.iceberg.hadoop.HadoopTables; import org.apache.iceberg.io.FileIO; import org.apache.iceberg.io.OutputFile; +import org.apache.iceberg.io.OutputFileFactory; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Iterables; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.spark.SparkCatalog; import org.apache.iceberg.spark.TestBase; import org.apache.iceberg.spark.source.ThreeColumnRecord; @@ -848,6 +856,87 @@ public void testDeleteFileSizeInBytesAfterRewrite() throws Exception { } } + // Regression test: a single Puffin file can hold multiple DVs (one blob per data file) + // referenced by distinct DeleteFile entries that share the same location. The rewrite must + // rewrite + // the physical Puffin file once (rather than colliding on the staging path) and stamp the + // rewritten size into every referencing manifest entry. + @TestTemplate + public void testSharedPuffinDeleteFileSizeAfterRewrite() throws Exception { + assumeThat(formatVersion) + .as("DVs are introduced in v3; v4 writes parquet manifests the test setup cannot read") + .isEqualTo(3); + + Table tableWithDVs = + createTableWithSnapshots( + tableDir.toFile().toURI().toString().concat("tableWithSharedPuffin"), 2); + + List dataFilePaths = Lists.newArrayList(); + tableWithDVs + .snapshots() + .forEach( + snapshot -> + snapshot + .addedDataFiles(tableWithDVs.io()) + .forEach(dataFile -> dataFilePaths.add(dataFile.location()))); + assertThat(dataFilePaths).as("Expected two data files to back two DVs").hasSize(2); + + List dvs = writeDVsForDataFiles(tableWithDVs, dataFilePaths); + assertThat(dvs) + .as("Both DVs should live in a single Puffin file") + .hasSize(2) + .allSatisfy(dv -> assertThat(dv.location()).isEqualTo(dvs.get(0).location())); + + RowDelta rowDelta = tableWithDVs.newRowDelta(); + dvs.forEach(rowDelta::addDeletes); + rowDelta.commit(); + + RewriteTablePath.Result result = + actions() + .rewriteTablePath(tableWithDVs) + .stagingLocation(stagingLocation()) + .rewriteLocationPrefix(tableWithDVs.location(), targetTableLocation()) + .execute(); + assertThat(result.rewrittenDeleteFilePathsCount()) + .as("Two DVs are two delete files with rewritten paths") + .isEqualTo(2); + copyTableFiles(result); + + Table targetTable = TABLES.load(targetTableLocation()); + Set rewrittenLocations = Sets.newHashSet(); + for (ManifestFile manifest : targetTable.currentSnapshot().deleteManifests(targetTable.io())) { + try (ManifestReader reader = + ManifestFiles.readDeleteManifest(manifest, targetTable.io(), targetTable.specs())) { + for (DeleteFile df : reader) { + rewrittenLocations.add(df.location()); + long actualSize = targetTable.io().newInputFile(df.location()).getLength(); + assertThat(df.fileSizeInBytes()) + .as("file_size_in_bytes should match the rewritten Puffin size for %s", df.location()) + .isEqualTo(actualSize); + } + } + } + assertThat(rewrittenLocations) + .as("Both DVs should point at the single rewritten Puffin file") + .hasSize(1); + } + + // Writes one DV per data file path into a single Puffin file, returning the resulting DeleteFiles + // (which share a location but carry distinct blob offsets). + private List writeDVsForDataFiles(Table targetTable, List dataFilePaths) + throws IOException { + OutputFileFactory fileFactory = + OutputFileFactory.builderFor(targetTable, 1, 1).format(FileFormat.PUFFIN).build(); + DVFileWriter writer = new BaseDVFileWriter(fileFactory, p -> null); + try (DVFileWriter closeableWriter = writer) { + for (String path : dataFilePaths) { + closeableWriter.delete(path, 0L, targetTable.spec(), (StructLike) null); + } + } + + return writer.result().deleteFiles(); + } + @TestTemplate public void testEqualityDeletes() throws Exception { Table sourceTable = createTableWithSnapshots(newTableLocation(), 1);