Core: update manifest delete file size after rewrite table action by mbutrovich · Pull Request #15470 · apache/iceberg

mbutrovich · 2026-02-27T17:12:50Z

Which issue does this PR close?

Rationale for this change

rewriteTablePath rewrites position delete files (updating embedded data file paths), which changes their size. file_size_in_bytes in the rewritten manifest reflects the original size, not the rewritten size. Readers that trust this field (Trino, Impala, Comet, iceberg-rust) fail with errors like "end of stream not reached."

What changes are included in this PR?

The position delete rewrite is a dedicated distributed phase, separate from manifest rewriting:

Enumerate the distinct position delete files referenced by the delete manifests being rewritten (metadata-only), deduped by identity so a file shared across manifests is handled once.
Rewrite those files in parallel, repartitioned by delete-file count and further deduped by location() so a Puffin file holding multiple DVs is written once. Each task measures its rewritten file's size from the writer (writer.length()) and emits path -> size; the driver collects this into a map and broadcasts it.
Rewrite manifests (metadata-only), stamping file_size_in_bytes from the broadcast map keyed by location. Entries absent from the map (for example DELETED entries not copied to the target) keep their original size.

Parallelism is at delete-file granularity. Sizes are measured on the workers from closed files, so there is no end-of-job burst of getLength()/HEAD calls and no in-progress-write length race.

API surface

The new methods are additive and the changed methods are only deprecated (no removals or signature changes), so this should not require .palantir/revapi.yml changes. Confirm with ./gradlew revapi.

RewriteTablePathUtil.rewriteDeleteManifest: new 10-param overload takes the measured Map<String, Long> of rewritten delete-file sizes and is metadata-only. The 9-param overload is @Deprecated (since 1.12.0, removed in 1.13.0) and delegates with an empty map.
RewriteTablePathUtil.rewritePositionDelete: new public method that rewrites a position delete file and returns the rewritten size, measured from the writer after close. The existing void rewritePositionDeleteFile is @Deprecated (since 1.12.0, removed in 1.13.0) and delegates to it.

Applied to Spark 3.5 / 4.0 / 4.1 together. Flink and Delta callers use the still-present deprecated overloads and are unaffected.

How are these changes tested?

New tests in TestRewriteTablePathsAction (all three Spark modules):

testDeleteFileSizeInBytesAfterRewrite: position deletes under a deeply nested path; asserts file_size_in_bytes in the rewritten manifest matches the on-disk size.
testSharedDeleteFileSizeAcrossManifests: same delete file referenced from multiple manifests; asserts size correctness across all rewritten delete manifests.
testMultipleDistinctDeleteFileSizesAfterRewrite: two distinct delete files of different sizes in one manifest; guards the per-path size mapping.
testSharedPuffinDeleteFileSizeAfterRewrite: two DVs packed in one Puffin file; regression test that the physical file is deduped by location (no AlreadyExistsException) and the rewritten size is stamped into each entry.

TestRewriteTablePathUtil adds testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries: a delete manifest with one live and one DELETED position delete entry, asserting the DELETED entry (absent from the measured-size map) keeps its original size.

AI Usage

I am more familiar with the iceberg-rust codebase, so Claude helped me navigate the code, prototype a design, and draft the PR description (in DataFusion Comet's PR template). Claude also helped me with the API change failures in CI.

anuragmantri · 2026-02-27T21:40:55Z

+  "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"


Was this moved unintentionally?

It's just what the ./gradlew revapi generated for me. As best as I can tell, this is what it did:

The existing 1.10.0 block moved earlier in the file (alphabetical sort?)

YAML indent changed from - code: (4-space) to - code: (2-space)

Our 4 new entries added (java.element.noLongerDeprecated, two java.method.numberOfParametersChanged, one java.method.removed for rewriteDeleteManifest)

Could you keep only the four new entries and revert the reordering/re-indentation of the existing 1.10.0 block? Manually adding the new entries into the existing block (keeping the original 4-space indent and ordering) would make the diff much smaller and easier to review.

I just want to mention that we must avoid breaking the API here. Please follow https://iceberg.apache.org/contribute/#minor-version-deprecations-required to avoid breaking the API

steveloughran · 2026-04-10T09:31:32Z

hmmm. In #15586 I've been working on skipping the HEAD call within s3afs and azure abfs client openFile() calls, and had been thinking that the manifest data would be best as it'd save another 100 mS/file on what is often a critical path. We need accurate manifests for that.

steveloughran · 2026-04-10T09:33:38Z

+  // manifest was not updated. Readers that use file_size_in_bytes to elide a stat() call may
+  // fail.
+  @TestTemplate
+  public void testDeleteFileSizeInBytesAfterRewrite() throws Exception {


are there equivalents of this for the other operations which add files to the manifests? if so they'd be handy being added for regression testing there too -though not in this PR

steveloughran

reviewed again. I think changes within RewriteTablePathUtil can eliminate the need for a HEAD call with a v3 puffin file.

…ask, 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.

…ustification "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter for inline position delete rewriting; rewritePositionDeleteFile now returns file size to avoid getLength HEAD call".

steveloughran

I like the change.

wypoon · 2026-04-15T21:20:56Z

@mbutrovich as you're changing the signature of RewriteTablePathUtil.rewriteDeleteManifest (in core), and it is used by Spark, Flink, and Delta conversion, you need to update all the callers. Unfortunately, this means updating all supported Spark and Flink versions. Normally we make changes to the latest supported Spark or Flink version first when making a Spark or Flink change, and then backport to earlier versions in separate PRs, but in this case, it needs to be done all together.

mbutrovich · 2026-04-15T21:36:37Z

@mbutrovich as you're changing the signature of RewriteTablePathUtil.rewriteDeleteManifest (in core), and it is used by Spark, Flink, and Delta conversion, you need to update all the callers. Unfortunately, this means updating all supported Spark and Flink versions. Normally we make changes to the latest supported Spark or Flink version first when making a Spark or Flink change, and then backport to earlier versions in separate PRs, but in this case, it needs to be done all together.

Sounds good, thanks @wypoon! @anuragmantri suggested I do the change in one version first to make it easier for reviewers (reduce the diff), get consensus on the approach, and then once folks are good with it I can generate the changes for the other versions. I'm happy to do that at any time.

wypoon · 2026-04-15T21:57:11Z

@mbutrovich it is true that having changes for multiple versions of Spark and Flink clutters up a PR and makes reviewing more difficult. However, you can at least put up the changes for one version of Flink and for Delta conversion, and then later for all the versions.
I haven't made changes to the GitHub actions before, but I wonder if you changed, e.g., https://github.com/apache/iceberg/blob/main/.github/workflows/spark-ci.yml#L80 to run only for 4.1 in the PR, if it affects the CI for the PR. You can try it (changing the workflows as a temporary measure, to be reverted in a final version) and see if the CI succeeds.

…e APIs per PR feedback

mbutrovich · 2026-06-22T16:26:36Z

Thanks for the detailed pass, @amogh-jahagirdar and @kevinjqliu. Sorry for the delayed response, I was on vacation for a few days. I reworked the PR per the latest feedback. API is now additive plus deprecation (9-param rewriteDeleteManifest and the void rewritePositionDeleteFile deprecated for 1.12.0/1.13.0; new public rewritePositionDelete), so ./gradlew revapi is clean. Fixed the DV-sharing-one-Puffin collision by deduping the physical rewrite on location() while keeping the logical delete-file count via DeleteFileSet.of. Added regression tests for the DV case and the DELETED-entry size fallback, and ported all the new tests across Spark 3.5 / 4.0 / 4.1. One note: the DV test is pinned to v3 because on v4 the test setup hits a parquet-manifest read path the harness does not support, which looks like a separate v4 gap unrelated to this fix.

steveloughran

looked at the code; all LGTM. that leave's @kevinjqliu's concerns about duplicate file writes to the same path.

mbutrovich · 2026-06-22T16:39:23Z

looked at the code; all LGTM. that leave's @kevinjqliu's concerns about duplicate file writes to the same path.

Thanks @steveloughran. @kevinjqliu's duplicate-write concern is fixed in bbbe466: the rewrite phase dedupes physical files by location() (RewriteTablePathSparkAction.java#L758-L769), so a Puffin file holding multiple DVs is written once while each manifest entry still gets the right size. Regression test: testSharedPuffinDeleteFileSizeAfterRewrite.

…r_rewrite # Conflicts: # core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java

# Conflicts: # core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java

kevinjqliu

LGTM, thank you for the comprehensive change and tests 🙏

kevinjqliu · 2026-06-27T18:14:51Z

cc @amogh-jahagirdar @szehon-ho please take a look when you get a chance

kevinjqliu · 2026-06-27T18:18:48Z

Codex flagged a similar issue with rewrite table action not updating the manifest file size in manifest lists 😄 (similar issue, just up one level). I think someone mentioned it somewhere in this PR.

Here are the relevant links:

rewrite_table_path does not update sizes of rewritten manifests in manifest lists #13719 / rewrite_table_path does not update manifest_length in rewritten manifest lists, causing Trino to fail with "Incorrect file size" #16905
Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists #16910

Just flagging since i think the fix would look very similar to this PR and we might want to include it in our patch release

amogh-jahagirdar

I think the fix looks good, after looking at it with fresh eyes I think we really should look at how to make the core APIs less cumbersome but that's for a follow on.

I do think the issue that @kevinjqliu pointed out is legitimate, but I think that can be fixed in a separate PR and included in the patches since this PR is fairly large already.

amogh-jahagirdar · 2026-06-27T20:34:41Z

+   *     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<DeleteFile> rewriteDeleteManifest(


Ok I don't think we need to do it in this PR but after looking at this, these APIs are getting pretty unfortunate, we're passing in 10 arguments now.

I think we'll want to introduce some builders and some structure like RewriteContext, and have structures which kick back things like a DeleteManifestRewriter.

I think what I'm asking here is more fundamental changes (and possibly an overhaul/elimination of this Util class) so like I said, definitely for something in the future.

lets track this #16983

amogh-jahagirdar · 2026-06-27T20:49:38Z

Thank you @mbutrovich and @kevinjqliu @steveloughran @szehon-ho @wypoon for reviewing. I'll go ahead and merge this

kevinjqliu · 2026-06-27T20:57:25Z

I'll take a look at #16910 implementation for the manifest file size in manifest lists issue

szehon-ho

Thanks for the fix — this is well-scoped and nicely tested. Two questions, no inline needed:

Enumerating live delete files vs. the old copy-plan set. The new flow enumerates live position-delete entries straight from the delete manifests (positionDeletesToRewrite), whereas the previous flow derived the files from the manifest-rewrite copy plan (which also pulled in DELETED entries via the unconditional toRewrite().add(...)). The net effect is that delete files appearing only as DELETED are no longer physically staged (an improvement), but rewrittenDeleteFilePathsCount can now differ from before. Since that's a user-visible result field, can you confirm the count change is intentional?
Broadcasting the size map. sparkContext().broadcast(rewrittenDeleteFileSizes) broadcasts a map with one entry per distinct delete-file location. The sizes are already collected to the driver, so it's bounded by that, but for tables with very many delete files this is still a sizable driver-collected + broadcast map. Could you add a short comment noting the scaling assumption (and confirm you've considered it for large delete-heavy tables)?

Edit: I think these are addressed already, no never mind!

…apache#15470)

github-actions Bot added spark core labels Feb 27, 2026

mbutrovich changed the title ~~Core: update delete file size after rewrite table action~~ Core: update manifest delete file size after rewrite table action Feb 27, 2026

anuragmantri reviewed Feb 27, 2026

View reviewed changes

mbutrovich force-pushed the delete_file_size_after_rewrite branch from 277c0e2 to 8922c70 Compare March 10, 2026 21:32

mbutrovich requested a review from anuragmantri March 12, 2026 15:10

anuragmantri reviewed Mar 13, 2026

View reviewed changes

Comment thread .palantir/revapi.yml Outdated

Comment thread core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java Outdated

steveloughran reviewed Apr 10, 2026

View reviewed changes

steveloughran reviewed Apr 14, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java Outdated

mbutrovich added 9 commits April 15, 2026 13:51

Fix API check?

2852745

Fix API check?

4b0bddd

Address PR feedback 2 and 4.

ceb65bc

Try to reduce revapi diff.

f98a692

Try to reduce revapi diff.

d0f9944

rebased on upstream main

517c018

Address PR feedback.

669e4f4

Update revapi. Used ./gradlew :iceberg-core:revapiAcceptAllBreaks --j…

78107a1

…ustification "rewriteDeleteManifest signature changed to accept PositionDeleteReaderWriter for inline position delete rewriting; rewritePositionDeleteFile now returns file size to avoid getLength HEAD call".

mbutrovich force-pushed the delete_file_size_after_rewrite branch from 58725c3 to 78107a1 Compare April 15, 2026 18:11

mbutrovich requested review from anuragmantri and steveloughran April 15, 2026 18:13

steveloughran reviewed Apr 15, 2026

View reviewed changes

steveloughran mentioned this pull request Apr 16, 2026

Core: Stream DV Puffin rewrite in RewriteTablePathUtil #15927

Closed

mbutrovich added 3 commits April 24, 2026 15:38

Create Spark 3.4, 3.5, 4.0 versions.

57b3c43

Merge branch 'main' into delete_file_size_after_rewrite

5f4ac53

Fix after upmerge with main.

e66e0b7

mbutrovich added 2 commits June 22, 2026 10:18

Merge branch 'main' into delete_file_size_after_rewrite

ecb1070

Core, Spark: dedupe DV rewrites by location and rename position-delet…

bbbe466

…e APIs per PR feedback

mbutrovich requested review from amogh-jahagirdar and kevinjqliu June 22, 2026 16:26

steveloughran reviewed Jun 22, 2026

View reviewed changes

Merge remote-tracking branch 'apache/main' into delete_file_size_afte…

1f3679f

…r_rewrite # Conflicts: # core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java

amogh-jahagirdar added this to the 1.10.3 milestone Jun 24, 2026

Merge branch 'main' into delete_file_size_after_rewrite

ec2484a

# Conflicts: # core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java

kevinjqliu approved these changes Jun 27, 2026

View reviewed changes

kevinjqliu requested a review from steveloughran June 27, 2026 18:14

kevinjqliu requested review from szehon-ho and removed request for steveloughran June 27, 2026 18:14

amogh-jahagirdar approved these changes Jun 27, 2026

View reviewed changes

amogh-jahagirdar merged commit 874e409 into apache:main Jun 27, 2026
54 checks passed

github-project-automation Bot moved this from In progress to Done in Comet Development Jun 27, 2026

kevinjqliu mentioned this pull request Jun 27, 2026

investigate better API structure for RewriteTablePathUtil #16983

Open

mbutrovich deleted the delete_file_size_after_rewrite branch June 29, 2026 13:19

szehon-ho reviewed Jun 29, 2026

View reviewed changes

jakelong95 pushed a commit to jakelong95/iceberg that referenced this pull request Jun 29, 2026

Core, Spark: Ensure correct delete file sizes in rewrite table action (…

a6489d2

…apache#15470)

szehon-ho mentioned this pull request Jun 30, 2026

Spark: Document broadcast size-map scaling in rewrite table action #17023

Closed

nssalian modified the milestones: 1.10.3, Iceberg 1.12.0 Jul 6, 2026

Kurtiscwright pushed a commit to Kurtiscwright/iceberg that referenced this pull request Jul 7, 2026

Core, Spark: Ensure correct delete file sizes in rewrite table action (…

9764e10

…apache#15470)

amogh-jahagirdar mentioned this pull request Jul 15, 2026

[1.10.x] Core, Spark: Ensure correct delete file sizes in rewrite table action #17215

Merged

Uh oh!

Conversation

mbutrovich commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

API surface

How are these changes tested?

AI Usage

Uh oh!

anuragmantri Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

mbutrovich Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

huaxingao May 12, 2026

Choose a reason for hiding this comment

Uh oh!

nastra May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveloughran commented Apr 10, 2026

Uh oh!

steveloughran Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

wypoon commented Apr 15, 2026

Uh oh!

mbutrovich commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wypoon commented Apr 15, 2026

Uh oh!

mbutrovich commented Jun 22, 2026

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

mbutrovich commented Jun 22, 2026

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Jun 27, 2026

Uh oh!

kevinjqliu commented Jun 27, 2026

Uh oh!

amogh-jahagirdar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jun 27, 2026

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Jun 27, 2026

Uh oh!

Uh oh!

kevinjqliu commented Jun 27, 2026

Uh oh!

szehon-ho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich commented Feb 27, 2026 •

edited

Loading

mbutrovich commented Apr 15, 2026 •

edited

Loading

amogh-jahagirdar left a comment •

edited

Loading

szehon-ho left a comment •

edited

Loading