RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3797892405
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -251,38 +251,77 @@ private static List<Snapshot> updatePathInSnapshots(
/**
* Rewrite a manifest list representing a snapshot, replacing path
references.
*
+ * <p>Every entry keeps its source {@code manifest_length}, which does not
match the rewritten
+ * manifest when the target prefix differs in length from the source.
Callers that rewrite to a
+ * different-length prefix should use {@link #rewriteManifestList(Snapshot,
FileIO, TableMetadata,
+ * List, Set, String, String, String, String, Map)} and pass the measured
lengths.
+ */
+ public static RewriteResult<ManifestFile> rewriteManifestList(
Review Comment:
Deprecate or even just completely remove. This code is wrong and corrupts
data. Our own readers will fail on manifest-lists produced by this method in
V4.
The behavior of "keeping the existing length" is wrong. We shouldn't support
it. If we wanted to keep this overload in a deprecated state I would argue we
should also be checking the lengths on disks of all the manifests in the
staging directory and assigning based on that. If the manifest isn't in the
staging directory we can fail. The deprecated version would be much less
performant but would at least be correct.
--
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]
