Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists

[wombatu-kun]

Closes #16905

Problem

RewriteTablePathUtil.rewriteManifestList updated only the manifest path in each manifest-list entry and left manifest_length (spec field 501) at the source value. A target prefix of a different length changes the embedded data-file paths, so the rewritten manifest differs in byte size. Readers that validate the field (Trino, Impala, iceberg-rust) then fail with Incorrect file size ... (end of stream not reached); Spark ignores it, which is why this only surfaces on some engines.

Fix

The rewritten manifest's length is only known once the manifests are rewritten, but the Spark action wrote the manifest list first. rebuildMetadata now reads each snapshot's manifest list once, rewrites the manifests while capturing each one's byte length from ManifestWriter.length(), and writes the manifest lists last, stamping manifest_length from those lengths. Reading the list once also lets the "manifest not under the source prefix" precondition run on the driver before anything is written, instead of after the manifests have been rewritten.

RewriteContentFileResult switches from Encoders.bean to Encoders.javaSerialization. The bean encoder derives an empty schema for that class, so its contents survive only because the optimizer collapses the serialize/deserialize pair in this plan shape; caching or shuffling the mapped Dataset would silently drop them, and a dropped length map would silently reinstate the source length. Measured on Spark 4.1.3: no added cost, since the collapsed plan serializes through neither encoder.

Known gap: incremental runs

A manifest carried over from an earlier increment is not rewritten again, so it keeps its source length and the entry stays inconsistent. Re-measuring it does not help: the file the earlier run wrote depends on the table metadata as it was then, and the manifest header embeds the schema, so re-measuring after schema evolution gives a different length (measured: +89 bytes for one added column). Closing this needs either read access to the target, which this action deliberately does not have, or state carried between runs. Flagged in the javadoc, left for a follow-up.

Tests

• testManifestLengthAfterRewrite: multi-snapshot table rewritten to a longer prefix, asserting manifest_length against the on-disk size for every manifest-list entry across current and historical snapshots, format versions 2-4 (Avro and Parquet manifests). Reverting the stamping fails it: expected 7587 but was 7516 (v2), 8406/8334 (v3), 12367/12191 (v4).
• assertRewriteChangedManifestLength keeps that assertion from degenerating into a tautology by requiring the prefix change to actually move a manifest's byte size.
• testDeleteFileSizeInBytesAfterRewrite now also asserts manifest_length for the rewritten delete manifest.
• TestRewriteTablePathUtil: one measured and one unmeasured manifest in a single list, pinning the stamping and that the map is keyed by the source path, plus source-prefix validation.

Notes

Same class of bug as the merged #15470 (delete-file file_size_in_bytes inside manifests), but for manifest_length in the manifest list. This builds on it.

Thanks @vaultah for the incremental-mode catch. You were right; it is now named as a known gap instead of being presented as intended behaviour.

---

AI Disclosure

• Model: Claude Opus 5
• Platform/Tool: Claude Code
• Human Oversight: fully reviewed
• Prompt Summary: Fix rewrite_table_path leaving the source manifest_length in the rewritten manifest lists.

[leeyam24]

Note on performance: this loop currently defaults to single-threaded execution because PySpark/SQL cannot configure an Executor Service, directly causing slower runtimes for rewrite_table_path. I've raised a feature request to address this, and I believe it's an important bottleneck we should advocate to fix.

[wombatu-kun]

The threading here is unchanged by this PR - both the new discovery loop and the manifest-list loop reuse the action's existing executorService, the same as the original single loop did. Making that executor parallel by default is orthogonal to the manifest_length fix, so #16752 is the right place to address it.

[leeyam24]

Of course.
I assumed it was Out-Of-Scope, just wanted to point out a potential bottleneck.

[leeyam24]

I'm done reviewing, looks pretty good to me.

[kevinjqliu]

Thanks for the PR! I'll take a look. I reference this in #15470 (comment)

I think it would be good to include this in the upcoming patch release.

Could you rebase to resolve conflict? Might also be good to follow the pattern from #15470

[wombatu-kun]

Rebased onto main; the conflict with #15470 is resolved. The manifest_length fix follows the same pattern as #15470 - new overloads that record the measured length via writer.length(), threaded through a Map<String, Long> with a getOrDefault fallback, and @Deprecated on the prior signatures. Ready for another look.

[vaultah]

This seems like an intentional decision, based on

Manifests not rewritten in an incremental run keep their original length.

but it does still mean that this procedure will silently produce incorrect manifest lists in incremental mode, if rewritten manifest lists referenced manifests that were already rewritten in a previous run.

If the rewritten manifest size changed after the prefix rewrite, which it typically does, the new manifest list is still inconsistent with the file it references.

This is a known incorrect behaviour described in #13720 (comment) and acknowledged there.

[wombatu-kun]

You're right - this is the same bug as your #13719 (#16905 is a later duplicate), and your #13720 went further than this PR: by rewriting every referenced manifest and requiring all lengths to be known, it kept the incremental case correct too. This PR keeps the incremental optimization (only new manifests are rewritten), so carried-over manifests fall back to file.length() and their manifest_length stays stale - the exact A/B/C case you raised with @dramaticlly. So the non-incremental path is fixed here but the incremental path is not, and closing it means re-rewriting all referenced manifests, the efficiency tradeoff that stalled #13720 at the community sync. I'd rather settle that completeness-vs-efficiency question on #13719 / dev@ than decide it unilaterally here, and I'm glad to help revive your #13720. Even as-is this PR is a strict improvement over main, where every entry kept the source length.

[wombatu-kun]

You're right, incremental runs are still inconsistent for carried-over manifests. This PR is now scoped to the complete-copy path, and the javadoc names the incremental case as a known gap rather than presenting it as intended behaviour. Leaving it for a follow-up.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.