wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3510091930
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -301,6 +334,8 @@ public static RewriteResult<ManifestFile>
rewriteManifestList(
for (ManifestFile file : manifestFiles) {
ManifestFile newFile = file.copy();
((StructLike) newFile).set(0, newPath(newFile.path(), sourcePrefix,
targetPrefix));
+ ((StructLike) newFile)
+ .set(1, rewrittenManifestLengths.getOrDefault(file.path(),
file.length()));
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
