varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3792257214
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -251,38 +251,113 @@ 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(
+ Snapshot snapshot,
+ FileIO io,
+ TableMetadata tableMetadata,
+ Set<String> manifestsToRewrite,
+ String sourcePrefix,
+ String targetPrefix,
+ String stagingDir,
+ String outputPath) {
+ // no lengths are known to this overload, so every entry keeping its
source length is expected
+ // and is not reported
+ return writeManifestList(
+ snapshot,
+ io,
+ tableMetadata,
+ manifestsInSnapshot(snapshot, io, sourcePrefix),
+ manifestsToRewrite,
+ sourcePrefix,
+ targetPrefix,
+ stagingDir,
+ outputPath,
+ ImmutableMap.of());
+ }
+
+ /**
+ * Rewrite a manifest list representing a snapshot, replacing path
references.
+ *
+ * <p>Each entry's {@code manifest_length} is taken from {@code
rewrittenManifestLengths}, because
+ * replacing the path prefix changes the byte length of the rewritten
manifest.
+ *
+ * <p>Known gap: an entry whose manifest is absent from the map keeps its
source length, which
+ * does not match the rewritten file. This is reachable in an incremental
run, where a manifest
+ * carried over from an earlier run is still referenced by the new
snapshot's manifest list but is
+ * not rewritten again. Re-measuring it here does not help, because the
length of the file the
+ * earlier run produced depends on the table metadata as it was then: the
manifest header embeds
+ * the current schema, so re-measuring after schema evolution yields a
different length. Closing
+ * the gap needs either read access to the target, which this action
deliberately does not have,
+ * or state carried between runs.
+ *
* @param snapshot snapshot represented by the manifest list
* @param io file io
* @param tableMetadata metadata of table
+ * @param manifestFiles the manifests referenced by the snapshot's manifest
list, as returned by
+ * {@link #manifestsInSnapshot(Snapshot, FileIO, String)}
* @param manifestsToRewrite a list of manifest files to filter for rewrite
* @param sourcePrefix source prefix that will be replaced
* @param targetPrefix target prefix that will replace it
* @param stagingDir staging directory
* @param outputPath location to write the manifest list
+ * @param rewrittenManifestLengths map from source manifest path to the byte
length of its
+ * rewritten manifest
* @return a copy plan for manifest files whose metadata were contained in
the rewritten manifest
* list
*/
public static RewriteResult<ManifestFile> rewriteManifestList(
Snapshot snapshot,
FileIO io,
TableMetadata tableMetadata,
+ List<ManifestFile> manifestFiles,
Set<String> manifestsToRewrite,
String sourcePrefix,
String targetPrefix,
String stagingDir,
- String outputPath) {
+ String outputPath,
+ Map<String, Long> rewrittenManifestLengths) {
+ manifestFiles.stream()
+ .filter(file -> rewrittenManifestLengths.get(file.path()) == null)
+ .forEach(
+ file ->
+ LOG.warn(
+ "No measured length for manifest {}; recording its source
length {}, which "
+ + "will not match the rewritten file at the target",
+ file.path(),
+ file.length()));
+
+ return writeManifestList(
Review Comment:
I am not feeling really good about this — 2 overloaded `rewriteManifestList`
functions further calling a 3rd `writeManifestList`, specifically for the log
warning.
I understand you did this so the original 8-arg function doesn't put
everything into log warns.
If we agree to put `@Deprecated` on the 8-arg, it can just call the 10-arg
with ImmutableMap.of() and warn on all paths — just like the previous
implementation, only without the isEmpty check.
Nit : And instead of one warning per manifest, we could aggregate to a
single line per manifest list (e.g. "N of M manifests keep their source
length"), so incremental runs don't pile up a warning per file per snapshot.
Not blocking from my side — happy to go either way if others feel
differently.
--
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]
