You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "wombatu-kun (via GitHub)" <gi...@apache.org> on 2026/06/21 13:59:22 UTC
[PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
wombatu-kun opened a new pull request, #16910:
URL: https://github.com/apache/iceberg/pull/16910
Closes #16905
## Problem
`RewriteTablePathUtil.rewriteManifestList` rewrote each manifest-list entry updating only the manifest path and left `manifest_length` (spec field 501) at the source value. When the target prefix differs in length from the source, the rewritten manifest embeds different-length data-file paths and therefore differs in byte size, so `manifest_length` no longer matches the rewritten manifest on disk. Readers that validate the field (Trino, Impala, iceberg-rust) fail with `Incorrect file size ... (end of stream not reached)`; Spark ignores the field, which is why the bug only surfaces on some engines.
## Fix
The rewritten manifest's true length is only known after the manifests are rewritten, but the Spark action wrote the manifest list first. `rebuildMetadata` now discovers the manifests to rewrite (read-only), rewrites them while capturing each one's byte length, and writes the manifest lists last, stamping `manifest_length` from the captured lengths. The length is taken from the manifest writer (`ManifestWriter.length()`) rather than a `getLength()`/HEAD call, so it stays accurate even where the length of an in-progress write underreports. Manifests not rewritten in an incremental run keep their original length. Because the staged metadata is copied to the target byte-for-byte (Distcp), the recorded length equals the target file's actual size.
## Tests
- `TestRewriteTablePathsAction#testManifestLengthAfterRewrite`: rewrites a table to a longer target prefix and asserts every rewritten manifest-list entry's `manifest_length` matches the on-disk manifest size (format versions 2-4, covering both Avro and Parquet manifests). Fails before this change, passes after.
- `TestRewriteTablePathUtil`: unit coverage for stamping `manifest_length` from the length map and for falling back to the source length when a manifest is absent from the map.
## Notes
Same class of bug as #15470 (delete-file `file_size_in_bytes` inside manifests), but for `manifest_length` in the manifest list; the two fixes are independent. They touch overlapping code in the rewrite path, so whichever lands second will need a small rebase.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3792199066
##########
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:
Sure, let's see what committers think.
I guess as this function returns stale lengths even when we have the updated is not the function we would like to keep.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3818131423
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
Done 6f73ff1da.
@varun-lakhyani over the manifests this method iterates the two are the same predicate: `rebuildMetadata` filters `snapshot.allManifests` with that set, and every manifest it then rewrites records its length on `RewriteResult`, so the only way into the map is having been rewritten. Carried-over manifests stay outside it by design and keep their 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791833720
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -334,6 +344,25 @@ private Result rebuildMetadata() {
manifestFiles,
sparkContext().broadcast(rewrittenDeleteFileSizes));
+ // rebuild manifest-list files last, stamping manifest_length with the rewritten manifest sizes
Review Comment:
Done 90d25bf7d
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
Not sure what's more important for now and moving forward
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5304444833
@wombatu-kun I think separating spark v3.5, v4.0 changes as backport afterwards will be good and make it easy for reviewers
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3810830100
##########
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
Review Comment:
Moved it to RewriteTablePath#startVersion in 4b8369ee1
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
Not sure what's more important going forward, maybe v4 needs letting go of incremental till we somehow fix it as current support is already not 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3790168810
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
Just wondering - did you consider carrying the length on RewriteResult instead of returning a Pair? it would merge in append the same way RewriteContentFileResult does, and rewriteDataManifest / rewriteDeleteManifest could keep their names and signatures — no new methods or deprecations.
Might not be worth at this point, just wondering if it was considered.
similar to https://github.com/apache/iceberg/pull/13720
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814464259
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
I think there is an issue in dropping Set.
We are iterating over `List<ManifestFile> manifestFiles = manifestFilesInSnapshot(io, snapshot);` which is source of manifest that might not have updated length in the map and only thing helping us fail hard using **`precondition`** is Set manifestsToRewrite which contains all that should have new length and map which contains all that has recorded new length.
We are iterating through all as there could be manifests which not need rewrite but we have to include them anyway
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802096917
##########
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:
Done c0337bea8
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3810834438
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
Done 4b8369ee1, scoped to manifestsToRewrite as in your second comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5234611811
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "vaultah (via GitHub)" <gi...@apache.org>.
vaultah commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3509271140
##########
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:
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 https://github.com/apache/iceberg/pull/13720#issuecomment-3235133985 and acknowledged there.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3747027281
##########
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, 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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5397431850
For follow ups we probably need to do the following
1. Disable incremental mode on V4, it will never be correct. We can do this at the action level. We should fail and fail hard.
2. Add a warning for non-incremental mode on V4 that users should switch to using relative paths.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3849105403
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -376,4 +386,121 @@ private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, Dele
return writer.toManifestFile();
}
+
+ @TestTemplate
+ public void testRewriteManifestListStampsMeasuredManifestLengths() throws IOException {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ table.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot snapshot = table.currentSnapshot();
+ List<ManifestFile> manifests = snapshot.allManifests(table.io());
+ assertThat(manifests).hasSize(2);
+ ManifestFile measured = manifests.get(0);
+ ManifestFile unmeasured = manifests.get(1);
+
+ String manifestPath = measured.path();
+ String sourcePrefix = manifestPath.substring(0, manifestPath.lastIndexOf("/metadata/"));
+ String targetPrefix = sourcePrefix + "/relocated";
+ String stagingDir = temp.resolve("staging").toString();
+ String outputPath = temp.resolve("rewritten-list-" + System.nanoTime() + ".avro").toString();
+
+ long rewrittenLength = measured.length() + 4242L;
+
+ RewriteTablePathUtil.RewriteResult<ManifestFile> result =
+ RewriteTablePathUtil.rewriteManifestList(
+ snapshot,
+ table.io(),
+ table.ops().current(),
+ ImmutableMap.of(measured.path(), rewrittenLength),
+ sourcePrefix,
+ targetPrefix,
+ stagingDir,
+ outputPath);
+
+ List<ManifestFile> rewritten = ManifestLists.read(table.io().newInputFile(outputPath));
+ assertThat(rewritten).hasSize(2);
+ Map<String, Long> lengthByPath =
+ rewritten.stream().collect(Collectors.toMap(ManifestFile::path, ManifestFile::length));
+
+ String measuredTargetPath =
+ RewriteTablePathUtil.newPath(measured.path(), sourcePrefix, targetPrefix);
+ String unmeasuredTargetPath =
+ RewriteTablePathUtil.newPath(unmeasured.path(), sourcePrefix, targetPrefix);
+ assertThat(lengthByPath)
+ .as("measured manifest should take the mapped length, keyed by its source path")
+ .containsEntry(measuredTargetPath, rewrittenLength);
+ assertThat(lengthByPath)
+ .as("unmeasured manifest should keep its source length")
+ .containsEntry(unmeasuredTargetPath, unmeasured.length());
+
+ assertThat(result.toRewrite())
+ .as("only manifests with a measured length should be rewritten")
+ .extracting(ManifestFile::path)
+ .containsExactly(measured.path());
+ assertThat(result.copyPlan())
+ .as("the copy plan should cover the staged copy of that manifest only")
+ .containsExactly(
+ Pair.of(
+ RewriteTablePathUtil.stagingPath(measured.path(), sourcePrefix, stagingDir),
+ measuredTargetPath));
+ }
+
+ @TestTemplate
Review Comment:
Done fdb242220. Removed - that precondition predates this PR, so the test was unrelated coverage.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3792202775
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
Okay makes sense to me
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5325468670
@varun-lakhyani the trim is reverted in c0337bea8: removing the 8-argument `rewriteManifestList` leaves v3.5 and v4.0 uncompilable, so the fix has to land in all three trees together.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3797957741
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
Why would we care about breaking the SerialVersionUID? I can't think of a serialization boundary for this object that we want to preserve?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5325338491
> I think we should avoid adding new apis, and I think we can do so here by just changing the rewriteManifestList and nothing else.
>
> Ideally I think we would want to change RewriteResult so that we can keep the methods as they are, the revapi breaks from disrupting the existing APIs are I think warranted because they basically cannot be used correctly as is.
c0337bea8 takes that shape. Since the old signature is gone, the fix is back in all three Spark trees rather than 4.1 alone.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791840098
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -376,4 +385,70 @@ private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, Dele
return writer.toManifestFile();
}
+
+ /**
+ * One manifest is measured and one is not, in a single manifest list. The measured one must take
+ * the mapped length and the unmeasured one must keep its source length, which pins both the
+ * stamping and the fact that the map is keyed by the source path, not the rewritten one.
+ */
+ @TestTemplate
+ public void testRewriteManifestListStampsMeasuredManifestLengths() throws IOException {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ table.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot snapshot = table.currentSnapshot();
+ List<ManifestFile> manifests = snapshot.allManifests(table.io());
+ assertThat(manifests).hasSize(2);
+ ManifestFile measured = manifests.get(0);
+ ManifestFile unmeasured = manifests.get(1);
+
+ String manifestPath = measured.path();
+ String sourcePrefix = manifestPath.substring(0, manifestPath.indexOf("/metadata/"));
+ String targetPrefix = sourcePrefix + "/relocated";
+ String stagingDir = temp.resolve("staging").toString();
+ String outputPath = temp.resolve("rewritten-list-" + System.nanoTime() + ".avro").toString();
+
+ // a value distinct from the source length so we can tell it was applied
+ long rewrittenLength = measured.length() + 4242L;
+
+ RewriteTablePathUtil.rewriteManifestList(
+ snapshot,
+ table.io(),
+ table.ops().current(),
+ manifests,
+ Set.of(measured.path(), unmeasured.path()),
+ sourcePrefix,
+ targetPrefix,
+ stagingDir,
+ outputPath,
+ Map.of(measured.path(), rewrittenLength));
Review Comment:
Done 90d25bf7d
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791838652
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -340,6 +418,8 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
* @param sourcePrefix source prefix that will be replaced
* @param targetPrefix target prefix that will replace it
* @return a copy plan of content files in the manifest that was rewritten
+ * @see #rewriteDataManifestAndMeasureLength which also returns the rewritten manifest length, for
+ * callers that need to record manifest_length in the manifest list
*/
public static RewriteResult<DataFile> rewriteDataManifest(
Review Comment:
Same answer as the rewriteManifestList thread: @Deprecated commits core to a removal release, which is a committer's call rather than part of a bug fix. The javadoc already points at the overload that returns the 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3792199066
##########
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:
Sure, I am fine with waiting to understand what commiters think.
I guess as this function returns stale lengths even when we have the updated is not the function we would like to keep.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3844841646
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -363,6 +363,96 @@ public void testFullTableRewritePath() throws Exception {
checkFileNum(3, 2, 2, 9, result);
}
+ @TestTemplate
+ public void testManifestLengthAfterRewrite() throws Exception {
Review Comment:
https://github.com/apache/iceberg/pull/17664/changes#diff-59aa6d10abf9d930dbfe52d740ebd436610721ee31e0b999c7bed6bf0c05cd3cR1807-R1813 maybe this could help
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
Not sure what's more important going forward
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814286278
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
On this point, maybe we should just change the API entirely so that we only take this map okf manifest to length. We only want to be able to rewrite entries with a length, so we may as well just drop the Set and just have Map<Manifest, Length> be the only thing we allow passing through. Then there is no ability to rewrite a manifest unless you also pass through a 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802064838
##########
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
Review Comment:
Done c0337bea8. Dropped.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3798078523
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
I think rewrite result is probably the wrong place to store it, but I also think having a pair with two very different types of return type is showing the strains on this api. It may make sense to try to just wrap OutputFile so that getLength is a noOp.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814464259
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
dropping set as is could harm us.
We are iterating over `List<ManifestFile> manifestFiles = manifestFilesInSnapshot(io, snapshot);` which is source of manifest that might not have updated length in the map and only thing helping us fail hard using **`precondition`** is Set manifestsToRewrite which contains all that should have new length and map which contains all that have recorded new length.
If we drop set we might need precondition some point else - while iteration -manifestFiles against the length map
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814286278
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
On this point, maybe we should just change the API entirely so that we only take this map of manifest to length. We only want to be able to rewrite entries with a length, so we may as well just drop the Set and just have Map<Manifest, Length> be the only thing we allow passing through. Then there is no ability to rewrite a manifest unless you also pass through a 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3818522845
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
Yeah I missed an important thing earlier - That precondition never belonged in function t**hat's called per snapshot** , when both Map and set used in it were **global (same everytime)**. It always checked the exact same thing for each snapshot.
Agreed to remove set.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802116052
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -340,6 +418,8 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
* @param sourcePrefix source prefix that will be replaced
* @param targetPrefix target prefix that will replace it
* @return a copy plan of content files in the manifest that was rewritten
+ * @see #rewriteDataManifestAndMeasureLength which also returns the rewritten manifest length, for
+ * callers that need to record manifest_length in the manifest list
*/
public static RewriteResult<DataFile> rewriteDataManifest(
Review Comment:
Not needed as of c0337bea8: `rewriteDataManifest` keeps its signature and records the rewritten length in the result.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "leeyam24 (via GitHub)" <gi...@apache.org>.
leeyam24 commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3458143162
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -321,6 +321,24 @@ private Result rebuildMetadata() {
.collect(Collectors.toCollection(DeleteFileSet::create));
rewritePositionDeletes(deleteFiles);
+ // rebuild manifest-list files last, stamping manifest_length with the rewritten manifest sizes
+ Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ Tasks.foreach(validSnapshots)
Review Comment:
Of course.
I assumed it was Out-Of-Scope, just wanted to point out a potential bottleneck.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "leeyam24 (via GitHub)" <gi...@apache.org>.
leeyam24 commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3451755566
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -321,6 +321,24 @@ private Result rebuildMetadata() {
.collect(Collectors.toCollection(DeleteFileSet::create));
rewritePositionDeletes(deleteFiles);
+ // rebuild manifest-list files last, stamping manifest_length with the rewritten manifest sizes
+ Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ Tasks.foreach(validSnapshots)
Review Comment:
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](https://github.com/apache/iceberg/issues/16752) to address this, and I believe it's an important bottleneck we should advocate to fix.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791830870
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -302,21 +302,31 @@ private Result rebuildMetadata() {
Set<Snapshot> validSnapshots =
Sets.difference(snapshotSet(endMetadata), snapshotSet(startMetadata));
- // rebuild manifest-list files
- Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ // Read every valid snapshot's manifest list once, before writing anything. The rewritten
+ // manifest list must record each referenced manifest's rewritten length (manifest_length),
+ // which is only known after the manifests are rewritten below.
+ Map<Snapshot, List<ManifestFile>> manifestsBySnapshot = Maps.newConcurrentMap();
Review Comment:
Done 90d25bf7d. The step list in the rebuildMetadata javadoc still described the old order, so the ordering now lives there instead.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791834206
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -494,12 +523,18 @@ private Set<Pair<String, String>> partitionStatsFileCopyPlan(
*
* @param snapshot snapshot represented by the manifest list
* @param tableMetadata metadata of table
+ * @param manifestFiles the manifests the snapshot's manifest list references
* @param manifestsToRewrite filter of manifests to rewrite.
+ * @param rewrittenManifestLengths map from source manifest path to its rewritten byte length
Review Comment:
Done 90d25bf7d
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791839403
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
Yes, and I implemented it to check. It works: rewriteDataManifest / rewriteDeleteManifest keep their signatures and stamp the length after the reduce, and append needs no change because it is set once rather than accumulated. What killed it is that adding a field to RewriteResult changes its default serialVersionUID, so revapi reports java.class.defaultSerializationChanged and the shape only lands with a permanent accepted-break entry in .palantir/revapi.yml. Two extra methods looked like the cheaper price, so I kept the Pair.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3799147600
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
I actually changed my mind, I think this is probably a good place to store the 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3798141089
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -552,10 +588,22 @@ private Set<String> manifestsToRewrite(
}
public static class RewriteContentFileResult extends RewriteResult<ContentFile<?>> {
+ // Map from source manifest path to the byte length of its rewritten manifest, used by the
+ // manifest-list rewrite to record an accurate manifest_length.
+ private final Map<String, Long> rewrittenManifestLengths = Maps.newHashMap();
+
+ public Map<String, Long> rewrittenManifestLengths() {
Review Comment:
nit but this should probably be an unmodifiable map. We are currently exposing this for read and write. So we have this pattern of
Result.getMap.put
Instead of
Result.addLength(manifestPath, length) Or what not
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814464259
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
I think there is an issue in dropping Set.
We are iterating over `List<ManifestFile> manifestFiles = manifestFilesInSnapshot(io, snapshot);` which is source of manifest that might not have updated length in the map and only thing helping us fail hard using **`precondition`** is Set manifestsToRewrite which contains all that should have new length and map which contains all that has recorded new 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3849109622
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -363,6 +363,96 @@ public void testFullTableRewritePath() throws Exception {
checkFileNum(3, 2, 2, 9, result);
}
+ @TestTemplate
+ public void testManifestLengthAfterRewrite() throws Exception {
+ String sourceLocation = newTableLocation();
+ Table sourceTable =
+ TABLES.create(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, String.valueOf(formatVersion)),
+ sourceLocation);
+
+ appendManifestLengthRecords(sourceLocation, 0);
+ appendManifestLengthRecords(sourceLocation, 1);
+ sourceTable.refresh();
+ assertThat(sourceTable.snapshots()).hasSize(2);
+
+ String targetLocation =
+ targetTableLocation() + "this/is/a/much/longer/nested/target/prefix/than/the/source";
+
+ RewriteTablePath.Result result =
+ actions()
+ .rewriteTablePath(sourceTable)
+ .stagingLocation(stagingLocation())
+ .rewriteLocationPrefix(sourceTable.location(), targetLocation)
+ .execute();
+ copyTableFiles(result);
+
+ Table targetTable = TABLES.load(targetLocation);
+ // snapshot 1 references its own manifest, snapshot 2 references both
+ assertThat(assertManifestLengthsMatchOnDisk(targetTable))
+ .as("should validate manifests across all (current and historical) snapshots")
+ .isEqualTo(3);
+ assertRewriteChangedManifestLength(sourceTable, targetTable);
+ }
+
+ private void appendManifestLengthRecords(String location, int batch) {
+ List<ThreeColumnRecord> records = Lists.newArrayList();
+ for (int i = 0; i < 30; i++) {
+ int id = batch * 100 + i;
+ records.add(new ThreeColumnRecord(id, "row-" + id, "data-" + id));
+ }
+ spark
+ .createDataFrame(records, ThreeColumnRecord.class)
+ .repartition(30)
+ .select("c1", "c2", "c3")
+ .write()
+ .format("iceberg")
+ .mode("append")
+ .save(location);
+ }
+
+ private int assertManifestLengthsMatchOnDisk(Table targetTable) {
+ FileIO io = targetTable.io();
+ int checkedManifests = 0;
+ for (Snapshot snapshot : targetTable.snapshots()) {
+ for (ManifestFile manifest : snapshot.allManifests(io)) {
+ assertThat(manifest.length())
+ .as(
+ "manifest_length in the rewritten manifest list of snapshot %s must match the"
+ + " on-disk size of %s",
+ snapshot.snapshotId(), manifest.path())
+ .isEqualTo(io.newInputFile(manifest.path()).getLength());
+ checkedManifests++;
+ }
+ }
+ return checkedManifests;
+ }
+
+ /** Guards the length assertions against a prefix change that moved no manifest's byte size. */
+ private void assertRewriteChangedManifestLength(Table sourceTable, Table targetTable) {
Review Comment:
Done fdb242220
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3844753705
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -376,4 +386,121 @@ private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, Dele
return writer.toManifestFile();
}
+
+ @TestTemplate
+ public void testRewriteManifestListStampsMeasuredManifestLengths() throws IOException {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ table.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot snapshot = table.currentSnapshot();
+ List<ManifestFile> manifests = snapshot.allManifests(table.io());
+ assertThat(manifests).hasSize(2);
+ ManifestFile measured = manifests.get(0);
+ ManifestFile unmeasured = manifests.get(1);
+
+ String manifestPath = measured.path();
+ String sourcePrefix = manifestPath.substring(0, manifestPath.lastIndexOf("/metadata/"));
+ String targetPrefix = sourcePrefix + "/relocated";
+ String stagingDir = temp.resolve("staging").toString();
+ String outputPath = temp.resolve("rewritten-list-" + System.nanoTime() + ".avro").toString();
+
+ long rewrittenLength = measured.length() + 4242L;
+
+ RewriteTablePathUtil.RewriteResult<ManifestFile> result =
+ RewriteTablePathUtil.rewriteManifestList(
+ snapshot,
+ table.io(),
+ table.ops().current(),
+ ImmutableMap.of(measured.path(), rewrittenLength),
+ sourcePrefix,
+ targetPrefix,
+ stagingDir,
+ outputPath);
+
+ List<ManifestFile> rewritten = ManifestLists.read(table.io().newInputFile(outputPath));
+ assertThat(rewritten).hasSize(2);
+ Map<String, Long> lengthByPath =
+ rewritten.stream().collect(Collectors.toMap(ManifestFile::path, ManifestFile::length));
+
+ String measuredTargetPath =
+ RewriteTablePathUtil.newPath(measured.path(), sourcePrefix, targetPrefix);
+ String unmeasuredTargetPath =
+ RewriteTablePathUtil.newPath(unmeasured.path(), sourcePrefix, targetPrefix);
+ assertThat(lengthByPath)
+ .as("measured manifest should take the mapped length, keyed by its source path")
+ .containsEntry(measuredTargetPath, rewrittenLength);
+ assertThat(lengthByPath)
+ .as("unmeasured manifest should keep its source length")
+ .containsEntry(unmeasuredTargetPath, unmeasured.length());
+
+ assertThat(result.toRewrite())
+ .as("only manifests with a measured length should be rewritten")
+ .extracting(ManifestFile::path)
+ .containsExactly(measured.path());
+ assertThat(result.copyPlan())
+ .as("the copy plan should cover the staged copy of that manifest only")
+ .containsExactly(
+ Pair.of(
+ RewriteTablePathUtil.stagingPath(measured.path(), sourcePrefix, stagingDir),
+ measuredTargetPath));
+ }
+
+ @TestTemplate
Review Comment:
How is this related to this change?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5307645907
Done 6e3e69199 - the PR is now core plus spark/v4.1 only. The 3.5 and 4.0 backport goes up as a separate PR once this one merges.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5308536826
@huaxingao @stevenzwu @dramaticlly As you guys were involved in older PR https://github.com/apache/iceberg/pull/13720 for this fix, Can you please take a look at this one.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802053661
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
Done c0337bea8. `RewriteResult` carries a map from source manifest path to rewritten length and merges it in `append`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3810842288
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
+ manifestFiles.stream()
+ .filter(mf -> !rewrittenManifestLengths.containsKey(mf.path()))
+ .count();
+ if (unmeasured > 0) {
+ LOG.warn(
Review Comment:
Done 4b8369ee1
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814464259
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
dropping set as is could harm us.
We are iterating over `List<ManifestFile> manifestFiles = manifestFilesInSnapshot(io, snapshot);` which is source of manifest that might not have updated length in the map and only thing helping us fail hard using **`precondition`** is Set manifestsToRewrite which contains all that should have new length and map which contains all that have recorded new length.
If we drop set we might need precondition some point else - while iteration -manifest list against the length map
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
Not sure what's more important going forward, maybe v4 needs letting go of incremental till we anyhow fix it as current support is already not 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3805620096
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
Something like
Preconditon
rewrittenManifestLengths.containsAll(manifestsToRewrite)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3849107476
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -363,6 +363,96 @@ public void testFullTableRewritePath() throws Exception {
checkFileNum(3, 2, 2, 9, result);
}
+ @TestTemplate
+ public void testManifestLengthAfterRewrite() throws Exception {
Review Comment:
Done fdb242220. assertManifestLengthsMatchOnDisk now runs after copyTableFiles in all 13 non-incremental tests and the dedicated test is gone.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3452736662
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -321,6 +321,24 @@ private Result rebuildMetadata() {
.collect(Collectors.toCollection(DeleteFileSet::create));
rewritePositionDeletes(deleteFiles);
+ // rebuild manifest-list files last, stamping manifest_length with the rewritten manifest sizes
+ Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ Tasks.foreach(validSnapshots)
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-4821641491
Thanks for the PR! I'll take a look. I reference this in https://github.com/apache/iceberg/pull/15470#issuecomment-4820017914
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5315422742
Just for reference — the flow before this PR and how it changes, so the correct manifest length is available at manifest-list write time.
<img width="6134" height="4513" alt="Untitled-2026-07-02-1819" src="https://github.com/user-attachments/assets/3a98734e-dd58-42da-8cbd-0686290aa50b" />
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5320822382
From architectural prospective. I think we should avoid adding new apis, and I think we can do so here by just changing the rewriteManifestList and nothing else.
I checked out @varun-lakhyani 's proposal as well,
It looks like Varun's pays a little bit in IO for each manifest which is pretty straight forward but straightforward
This Pr returns the length along with the result object but changes a some return signatures and adds new methods to do it.
Ideally I think we would want to change RewriteResult so that we can keep the methods as they are, the revapi breaks from disrupting the existing APIs are I think warranted because they basically cannot be used correctly as is.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3789976357
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -494,12 +523,18 @@ private Set<Pair<String, String>> partitionStatsFileCopyPlan(
*
* @param snapshot snapshot represented by the manifest list
* @param tableMetadata metadata of table
+ * @param manifestFiles the manifests the snapshot's manifest list references
* @param manifestsToRewrite filter of manifests to rewrite.
+ * @param rewrittenManifestLengths map from source manifest path to its rewritten byte length
Review Comment:
I would make these three same as rewriteManifestList in Util.
https://github.com/apache/iceberg/pull/16910/changes#diff-285b4520a0f058b7fe2a0103efe3f15c0021a6acd9031cdc22c3ddae6151b467R299-R307
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -302,21 +302,31 @@ private Result rebuildMetadata() {
Set<Snapshot> validSnapshots =
Sets.difference(snapshotSet(endMetadata), snapshotSet(startMetadata));
- // rebuild manifest-list files
- Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ // Read every valid snapshot's manifest list once, before writing anything. The rewritten
+ // manifest list must record each referenced manifest's rewritten length (manifest_length),
+ // which is only known after the manifests are rewritten below.
+ Map<Snapshot, List<ManifestFile>> manifestsBySnapshot = Maps.newConcurrentMap();
Tasks.foreach(validSnapshots)
.noRetry()
.throwFailureWhenFinished()
.executeWith(executorService)
.run(
snapshot ->
- manifestListResults.add(
- rewriteManifestList(snapshot, endMetadata, manifestsToRewrite)));
-
- RewriteResult<ManifestFile> rewriteManifestListResult = new RewriteResult<>();
- manifestListResults.forEach(rewriteManifestListResult::append);
-
- Set<ManifestFile> manifestFiles = rewriteManifestListResult.toRewrite();
+ manifestsBySnapshot.put(
+ snapshot,
+ RewriteTablePathUtil.manifestsInSnapshot(snapshot, table.io(), sourcePrefix)));
+
+ // Manifests selected for rewrite. In an incremental run this is only the manifests added by the
+ // delta snapshots, so a manifest carried over from an earlier run is not rewritten here and
+ // keeps its source length in the manifest list. See the note on rewriteManifestList.
+ Set<ManifestFile> manifestFiles = Sets.newHashSet();
Review Comment:
nit maybe code is self explaining here, if required this can be made one liner
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -351,17 +431,64 @@ public static RewriteResult<DataFile> rewriteDataManifest(
String sourcePrefix,
String targetPrefix)
throws IOException {
+ return rewriteDataManifestAndMeasureLength(
+ manifestFile,
+ snapshotIds,
+ outputFile,
+ io,
+ format,
+ specsById,
+ sourcePrefix,
+ targetPrefix)
+ .first();
+ }
+
+ /**
+ * Rewrite a data manifest, replacing path references, and return the rewritten manifest's byte
+ * length.
+ *
+ * <p>The length is read from the closed manifest writer rather than via a separate {@code
+ * getLength()} call, which would cost a stat request per manifest against object storage. Callers
+ * record this length as the {@code manifest_length} of the rewritten manifest in the manifest
+ * list.
+ *
+ * @param manifestFile source manifest file to rewrite
+ * @param snapshotIds snapshot ids for filtering returned data manifest entries
+ * @param outputFile output file to rewrite manifest file to
+ * @param io file io
+ * @param format format of the manifest file
+ * @param specsById map of partition specs by id
+ * @param sourcePrefix source prefix that will be replaced
+ * @param targetPrefix target prefix that will replace it
+ * @return the copy plan of content files in the rewritten manifest, paired with the rewritten
+ * manifest's byte length
+ */
+ public static Pair<RewriteResult<DataFile>, Long> rewriteDataManifestAndMeasureLength(
+ ManifestFile manifestFile,
+ Set<Long> snapshotIds,
+ OutputFile outputFile,
+ FileIO io,
+ int format,
+ Map<Integer, PartitionSpec> specsById,
+ String sourcePrefix,
+ String targetPrefix)
+ throws IOException {
PartitionSpec spec = specsById.get(manifestFile.partitionSpecId());
- try (ManifestWriter<DataFile> writer =
- ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(format, spec, outputFile, manifestFile.snapshotId());
+ RewriteResult<DataFile> result;
+ try (writer;
ManifestReader<DataFile> reader =
ManifestFiles.read(manifestFile, io, specsById).select(Arrays.asList("*"))) {
- return StreamSupport.stream(reader.entries().spliterator(), false)
- .map(
- entry ->
- writeDataFileEntry(entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
- .reduce(new RewriteResult<>(), RewriteResult::append);
+ result =
+ StreamSupport.stream(reader.entries().spliterator(), false)
+ .map(
+ entry ->
+ writeDataFileEntry(
+ entry, snapshotIds, spec, sourcePrefix, targetPrefix, writer))
+ .reduce(new RewriteResult<>(), RewriteResult::append);
}
+ return Pair.of(result, writer.length());
Review Comment:
Just wondering - did you consider carrying the length on RewriteResult instead of returning a Pair? it would merge in append the same way RewriteContentFileResult does, and rewriteDataManifest / rewriteDeleteManifest could keep their names and signatures — no new methods or deprecations.
Might not be worth at this point, just wondering if it was considered.
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -376,4 +385,70 @@ private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, Dele
return writer.toManifestFile();
}
+
+ /**
+ * One manifest is measured and one is not, in a single manifest list. The measured one must take
+ * the mapped length and the unmeasured one must keep its source length, which pins both the
+ * stamping and the fact that the map is keyed by the source path, not the rewritten one.
+ */
+ @TestTemplate
+ public void testRewriteManifestListStampsMeasuredManifestLengths() throws IOException {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ table.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot snapshot = table.currentSnapshot();
+ List<ManifestFile> manifests = snapshot.allManifests(table.io());
+ assertThat(manifests).hasSize(2);
+ ManifestFile measured = manifests.get(0);
+ ManifestFile unmeasured = manifests.get(1);
+
+ String manifestPath = measured.path();
+ String sourcePrefix = manifestPath.substring(0, manifestPath.indexOf("/metadata/"));
+ String targetPrefix = sourcePrefix + "/relocated";
+ String stagingDir = temp.resolve("staging").toString();
+ String outputPath = temp.resolve("rewritten-list-" + System.nanoTime() + ".avro").toString();
+
+ // a value distinct from the source length so we can tell it was applied
+ long rewrittenLength = measured.length() + 4242L;
+
+ RewriteTablePathUtil.rewriteManifestList(
+ snapshot,
+ table.io(),
+ table.ops().current(),
+ manifests,
+ Set.of(measured.path(), unmeasured.path()),
+ sourcePrefix,
+ targetPrefix,
+ stagingDir,
+ outputPath,
+ Map.of(measured.path(), rewrittenLength));
Review Comment:
nit
ImmutableMap just to be consistent?
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -334,6 +344,25 @@ private Result rebuildMetadata() {
manifestFiles,
sparkContext().broadcast(rewrittenDeleteFileSizes));
+ // rebuild manifest-list files last, stamping manifest_length with the rewritten manifest sizes
Review Comment:
nit
similar maybe just keep rebuild manifest-list files,
updated manifest file size is the base functionality we might not need to specify separately
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -302,21 +302,31 @@ private Result rebuildMetadata() {
Set<Snapshot> validSnapshots =
Sets.difference(snapshotSet(endMetadata), snapshotSet(startMetadata));
- // rebuild manifest-list files
- Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ // Read every valid snapshot's manifest list once, before writing anything. The rewritten
+ // manifest list must record each referenced manifest's rewritten length (manifest_length),
+ // which is only known after the manifests are rewritten below.
+ Map<Snapshot, List<ManifestFile>> manifestsBySnapshot = Maps.newConcurrentMap();
Review Comment:
nit. maybe just Read every valid snapshot's manifest list works.
i think the reason for reordering doesn't belong here
##########
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:
Can we make it like `@Deprecated` and `@deprecated` since 1.12.0, will be removed in 1.13.0; use the overload....
The stale-size rewriteDeleteManifest in the same file is `@Deprecated` for the same reason (#15470).
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -301,6 +340,15 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
for (ManifestFile file : manifestFiles) {
ManifestFile newFile = file.copy();
((StructLike) newFile).set(0, newPath(newFile.path(), sourcePrefix, targetPrefix));
+ Long rewrittenLength = rewrittenManifestLengths.get(file.path());
+ if (rewrittenLength == null && !rewrittenManifestLengths.isEmpty()) {
Review Comment:
I assume `!rewrittenManifestLengths.isEmpty()` is put just to ignore the opt out case of 8 params args rewriteManifestList but it silently skips the case where map is empty, maybe case where no manifest is selected for rewrite.
Maybe a boolean or something for the case when it's called via the older rewriteManifestList - or if that one gets `@Deprecated`, just let the empty map warn too and drop the special case.
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -376,4 +385,70 @@ private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, Dele
return writer.toManifestFile();
}
+
+ /**
+ * One manifest is measured and one is not, in a single manifest list. The measured one must take
+ * the mapped length and the unmeasured one must keep its source length, which pins both the
+ * stamping and the fact that the map is keyed by the source path, not the rewritten one.
+ */
+ @TestTemplate
+ public void testRewriteManifestListStampsMeasuredManifestLengths() throws IOException {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ table.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot snapshot = table.currentSnapshot();
+ List<ManifestFile> manifests = snapshot.allManifests(table.io());
+ assertThat(manifests).hasSize(2);
+ ManifestFile measured = manifests.get(0);
+ ManifestFile unmeasured = manifests.get(1);
+
+ String manifestPath = measured.path();
+ String sourcePrefix = manifestPath.substring(0, manifestPath.indexOf("/metadata/"));
Review Comment:
nit lastIndexOf instead of indexOf?
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -328,6 +376,36 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
return manifestFiles;
}
+ /**
+ * Read the manifests referenced by a snapshot's manifest list, without writing anything.
+ *
+ * <p>Callers need this before producing the rewritten manifest list, which records each
+ * referenced manifest's rewritten length. Reading once here and passing the result to {@link
+ * #rewriteManifestList(Snapshot, FileIO, TableMetadata, List, Set, String, String, String,
+ * String, Map)} also avoids reading the manifest list twice.
+ *
+ * <p>An unreadable manifest list is logged and treated as empty, matching the behaviour this
+ * method was extracted from.
+ *
+ * @param snapshot snapshot whose manifest list is read
+ * @param io file io
+ * @param sourcePrefix source prefix every referenced manifest must live under
+ * @return the manifests referenced by the snapshot's manifest list, or an empty list if the
+ * manifest list could not be read
+ */
+ public static List<ManifestFile> manifestsInSnapshot(
Review Comment:
Confusing with `manifestFilesInSnapshot`, but this one also validates via Preconditions and throws, and both stay.
Maybe something like `manifestFilesInSnapshotValidatingPrefix` so the difference is visible at the callsite?
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -340,6 +418,8 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
* @param sourcePrefix source prefix that will be replaced
* @param targetPrefix target prefix that will replace it
* @return a copy plan of content files in the manifest that was rewritten
+ * @see #rewriteDataManifestAndMeasureLength which also returns the rewritten manifest length, for
+ * callers that need to record manifest_length in the manifest list
*/
public static RewriteResult<DataFile> rewriteDataManifest(
Review Comment:
Should this get `@Deprecated` like the old 9-arg rewriteDeleteManifest?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791837912
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -328,6 +376,36 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
return manifestFiles;
}
+ /**
+ * Read the manifests referenced by a snapshot's manifest list, without writing anything.
+ *
+ * <p>Callers need this before producing the rewritten manifest list, which records each
+ * referenced manifest's rewritten length. Reading once here and passing the result to {@link
+ * #rewriteManifestList(Snapshot, FileIO, TableMetadata, List, Set, String, String, String,
+ * String, Map)} also avoids reading the manifest list twice.
+ *
+ * <p>An unreadable manifest list is logged and treated as empty, matching the behaviour this
+ * method was extracted from.
+ *
+ * @param snapshot snapshot whose manifest list is read
+ * @param io file io
+ * @param sourcePrefix source prefix every referenced manifest must live under
+ * @return the manifests referenced by the snapshot's manifest list, or an empty list if the
+ * manifest list could not be read
+ */
+ public static List<ManifestFile> manifestsInSnapshot(
Review Comment:
Done 90d25bf7d. manifestFilesInSnapshot had a single caller, so it is inlined and only manifestsInSnapshot remains.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
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.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-5416985357
Merged, Thanks @wombatu-kun for the PR and @varun-lakhyani for the review.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802036126
##########
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:
Done c0337bea8. Removed rather than deprecated. The break is recorded in `.palantir/revapi.yml`.
On "we shouldn't support it" more generally: a manifest this run rewrote can no longer fall back to its source length either, that now fails. The one case left is a manifest carried over from an earlier incremental run, which this run deliberately does not rewrite, so there is nothing to measure. Closing that means rewriting carried-over manifests again and re-copying them so the target file matches the recorded length, which is what #13720 proposed for #13719 and where its cost discussion stalled.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
Not sure what's more important going forward
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3805612206
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
I think basically we probably want to say that all rewrittenManifests must have a defined length. If a manifest path is changed, then it's length must also change. Anything else should hard fail
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3798020510
##########
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
Review Comment:
I'm not sure this additional java doc is very helpful. It's pretty deep in the API (an actual user will probably never get here.) We would want to keep this note if we actually wanted it in the RewriteTablePath API and not in this utility class. You can tell this is in the wrong spot because it's referring to "incremental runs" which don't actually exist in this utility class.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3798116895
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -328,6 +376,36 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
return manifestFiles;
}
+ /**
+ * Read the manifests referenced by a snapshot's manifest list, without writing anything.
+ *
+ * <p>Callers need this before producing the rewritten manifest list, which records each
+ * referenced manifest's rewritten length. Reading once here and passing the result to {@link
+ * #rewriteManifestList(Snapshot, FileIO, TableMetadata, List, Set, String, String, String,
+ * String, Map)} also avoids reading the manifest list twice.
+ *
+ * <p>An unreadable manifest list is logged and treated as empty, matching the behaviour this
+ * method was extracted from.
+ *
+ * @param snapshot snapshot whose manifest list is read
+ * @param io file io
+ * @param sourcePrefix source prefix every referenced manifest must live under
+ * @return the manifests referenced by the snapshot's manifest list, or an empty list if the
+ * manifest list could not be read
+ */
+ public static List<ManifestFile> manifestsInSnapshot(
Review Comment:
I'm also not sure why this is public and the java doc is extremely specific to it's current usage. That's another hint to me that this is not actually a public method. I feel like we are exposing public API to fix implementation details.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3849104021
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -599,7 +614,8 @@ private RewriteContentFileResult rewriteManifests(
sourcePrefix,
targetPrefix,
rewrittenDeleteFileSizes),
- Encoders.bean(RewriteContentFileResult.class))
+ // Encoders.bean derives an empty schema here and would drop the copy plan and lengths
Review Comment:
Done fdb242220
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#issuecomment-4828393323
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3844801719
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -363,6 +363,96 @@ public void testFullTableRewritePath() throws Exception {
checkFileNum(3, 2, 2, 9, result);
}
+ @TestTemplate
+ public void testManifestLengthAfterRewrite() throws Exception {
Review Comment:
We don't really nee individual tests for this. Every test we do that rewrites manifests should also check to see that the lengths are the same. You already have this in a helper assertManifestLengthsMatchOnDisk which we should just be calling on all of the tests (except Incremental rewrites). Then we can remove tests that are just checking this invariant which we expect to be true all the time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3844813341
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -363,6 +363,96 @@ public void testFullTableRewritePath() throws Exception {
checkFileNum(3, 2, 2, 9, result);
}
+ @TestTemplate
+ public void testManifestLengthAfterRewrite() throws Exception {
+ String sourceLocation = newTableLocation();
+ Table sourceTable =
+ TABLES.create(
+ SCHEMA,
+ PartitionSpec.unpartitioned(),
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, String.valueOf(formatVersion)),
+ sourceLocation);
+
+ appendManifestLengthRecords(sourceLocation, 0);
+ appendManifestLengthRecords(sourceLocation, 1);
+ sourceTable.refresh();
+ assertThat(sourceTable.snapshots()).hasSize(2);
+
+ String targetLocation =
+ targetTableLocation() + "this/is/a/much/longer/nested/target/prefix/than/the/source";
+
+ RewriteTablePath.Result result =
+ actions()
+ .rewriteTablePath(sourceTable)
+ .stagingLocation(stagingLocation())
+ .rewriteLocationPrefix(sourceTable.location(), targetLocation)
+ .execute();
+ copyTableFiles(result);
+
+ Table targetTable = TABLES.load(targetLocation);
+ // snapshot 1 references its own manifest, snapshot 2 references both
+ assertThat(assertManifestLengthsMatchOnDisk(targetTable))
+ .as("should validate manifests across all (current and historical) snapshots")
+ .isEqualTo(3);
+ assertRewriteChangedManifestLength(sourceTable, targetTable);
+ }
+
+ private void appendManifestLengthRecords(String location, int batch) {
+ List<ThreeColumnRecord> records = Lists.newArrayList();
+ for (int i = 0; i < 30; i++) {
+ int id = batch * 100 + i;
+ records.add(new ThreeColumnRecord(id, "row-" + id, "data-" + id));
+ }
+ spark
+ .createDataFrame(records, ThreeColumnRecord.class)
+ .repartition(30)
+ .select("c1", "c2", "c3")
+ .write()
+ .format("iceberg")
+ .mode("append")
+ .save(location);
+ }
+
+ private int assertManifestLengthsMatchOnDisk(Table targetTable) {
+ FileIO io = targetTable.io();
+ int checkedManifests = 0;
+ for (Snapshot snapshot : targetTable.snapshots()) {
+ for (ManifestFile manifest : snapshot.allManifests(io)) {
+ assertThat(manifest.length())
+ .as(
+ "manifest_length in the rewritten manifest list of snapshot %s must match the"
+ + " on-disk size of %s",
+ snapshot.snapshotId(), manifest.path())
+ .isEqualTo(io.newInputFile(manifest.path()).getLength());
+ checkedManifests++;
+ }
+ }
+ return checkedManifests;
+ }
+
+ /** Guards the length assertions against a prefix change that moved no manifest's byte size. */
+ private void assertRewriteChangedManifestLength(Table sourceTable, Table targetTable) {
Review Comment:
We don't really need to do this. The positive invariant (the new length matches) is enough.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791832997
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -302,21 +302,31 @@ private Result rebuildMetadata() {
Set<Snapshot> validSnapshots =
Sets.difference(snapshotSet(endMetadata), snapshotSet(startMetadata));
- // rebuild manifest-list files
- Set<RewriteResult<ManifestFile>> manifestListResults = Sets.newConcurrentHashSet();
+ // Read every valid snapshot's manifest list once, before writing anything. The rewritten
+ // manifest list must record each referenced manifest's rewritten length (manifest_length),
+ // which is only known after the manifests are rewritten below.
+ Map<Snapshot, List<ManifestFile>> manifestsBySnapshot = Maps.newConcurrentMap();
Tasks.foreach(validSnapshots)
.noRetry()
.throwFailureWhenFinished()
.executeWith(executorService)
.run(
snapshot ->
- manifestListResults.add(
- rewriteManifestList(snapshot, endMetadata, manifestsToRewrite)));
-
- RewriteResult<ManifestFile> rewriteManifestListResult = new RewriteResult<>();
- manifestListResults.forEach(rewriteManifestListResult::append);
-
- Set<ManifestFile> manifestFiles = rewriteManifestListResult.toRewrite();
+ manifestsBySnapshot.put(
+ snapshot,
+ RewriteTablePathUtil.manifestsInSnapshot(snapshot, table.io(), sourcePrefix)));
+
+ // Manifests selected for rewrite. In an incremental run this is only the manifests added by the
+ // delta snapshots, so a manifest carried over from an earlier run is not rewritten here and
+ // keeps its source length in the manifest list. See the note on rewriteManifestList.
+ Set<ManifestFile> manifestFiles = Sets.newHashSet();
Review Comment:
Done 90d25bf7d
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791834921
##########
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:
The <p> note above the method already routes callers to the overload that takes the lengths. I left @Deprecated off because it commits core to removing the method in a named release, which is a committer's call rather than something to fold into a bug fix - #15470's tag was set by that PR's author for their own method. Happy to add it if a committer wants it here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791840677
##########
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java:
##########
@@ -376,4 +385,70 @@ private ManifestFile deleteManifestWithLiveAndDeletedEntry(DeleteFile live, Dele
return writer.toManifestFile();
}
+
+ /**
+ * One manifest is measured and one is not, in a single manifest list. The measured one must take
+ * the mapped length and the unmeasured one must keep its source length, which pins both the
+ * stamping and the fact that the map is keyed by the source path, not the rewritten one.
+ */
+ @TestTemplate
+ public void testRewriteManifestListStampsMeasuredManifestLengths() throws IOException {
+ table.newFastAppend().appendFile(FILE_A).commit();
+ table.newFastAppend().appendFile(FILE_B).commit();
+ Snapshot snapshot = table.currentSnapshot();
+ List<ManifestFile> manifests = snapshot.allManifests(table.io());
+ assertThat(manifests).hasSize(2);
+ ManifestFile measured = manifests.get(0);
+ ManifestFile unmeasured = manifests.get(1);
+
+ String manifestPath = measured.path();
+ String sourcePrefix = manifestPath.substring(0, manifestPath.indexOf("/metadata/"));
Review Comment:
Done 90d25bf7d
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3791835569
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -301,6 +340,15 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
for (ManifestFile file : manifestFiles) {
ManifestFile newFile = file.copy();
((StructLike) newFile).set(0, newPath(newFile.path(), sourcePrefix, targetPrefix));
+ Long rewrittenLength = rewrittenManifestLengths.get(file.path());
+ if (rewrittenLength == null && !rewrittenManifestLengths.isEmpty()) {
Review Comment:
Done 90d25bf7d. The check moved out of the write loop into the overload that takes the map, so an empty map now warns for every entry, and the 8-arg overload is silent by construction rather than by the map happening to be empty.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3805592227
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
+ manifestFiles.stream()
+ .filter(mf -> !rewrittenManifestLengths.containsKey(mf.path()))
+ .count();
+ if (unmeasured > 0) {
+ LOG.warn(
Review Comment:
I don't think we should be warning, this is expected behavior? You could keep it as debug or info
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3844885306
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -599,7 +614,8 @@ private RewriteContentFileResult rewriteManifests(
sourcePrefix,
targetPrefix,
rewrittenDeleteFileSizes),
- Encoders.bean(RewriteContentFileResult.class))
+ // Encoders.bean derives an empty schema here and would drop the copy plan and lengths
Review Comment:
This feels unrelated to this change, and currently has no tests that break with it unchanged. I would change it back and have a follow up if we want to make a defensive change here. But as is I don't think this is actually broken.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #16910:
URL: https://github.com/apache/iceberg/pull/16910
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802073405
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -328,6 +376,36 @@ private static List<ManifestFile> manifestFilesInSnapshot(FileIO io, Snapshot sn
return manifestFiles;
}
+ /**
+ * Read the manifests referenced by a snapshot's manifest list, without writing anything.
+ *
+ * <p>Callers need this before producing the rewritten manifest list, which records each
+ * referenced manifest's rewritten length. Reading once here and passing the result to {@link
+ * #rewriteManifestList(Snapshot, FileIO, TableMetadata, List, Set, String, String, String,
+ * String, Map)} also avoids reading the manifest list twice.
+ *
+ * <p>An unreadable manifest list is logged and treated as empty, matching the behaviour this
+ * method was extracted from.
+ *
+ * @param snapshot snapshot whose manifest list is read
+ * @param io file io
+ * @param sourcePrefix source prefix every referenced manifest must live under
+ * @return the manifests referenced by the snapshot's manifest list, or an empty list if the
+ * manifest list could not be read
+ */
+ public static List<ManifestFile> manifestsInSnapshot(
Review Comment:
Done c0337bea8. `manifestsInSnapshot` is gone; the action uses the existing `Snapshot#allManifests(FileIO)`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3802083492
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java:
##########
@@ -552,10 +588,22 @@ private Set<String> manifestsToRewrite(
}
public static class RewriteContentFileResult extends RewriteResult<ContentFile<?>> {
+ // Map from source manifest path to the byte length of its rewritten manifest, used by the
+ // manifest-list rewrite to record an accurate manifest_length.
+ private final Map<String, Long> rewrittenManifestLengths = Maps.newHashMap();
+
+ public Map<String, Long> rewrittenManifestLengths() {
Review Comment:
Done c0337bea8
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814464259
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
dropping set as is could harm us.
We are iterating over `List<ManifestFile> manifestFiles = manifestFilesInSnapshot(io, snapshot);` which is source of manifest that might not have updated length in the map and only thing helping us fail hard using **`precondition`** is Set manifestsToRewrite which contains all that should have new length and map which contains all that have recorded new length.
If we drop set we might need precondition at some point else - while iteration -manifestFiles against the length map
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
Not sure what's more important going forward, Maybe fail hard could be the approach which suits v4.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Spark: Fix stale manifest_length in rewrite_table_path manifest lists [iceberg]
Posted by "varun-lakhyani (via GitHub)" <gi...@apache.org>.
varun-lakhyani commented on code in PR #16910:
URL: https://github.com/apache/iceberg/pull/16910#discussion_r3814535622
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -283,6 +301,19 @@ public static RewriteResult<ManifestFile> rewriteManifestList(
mf.path(),
sourcePrefix));
+ long unmeasured =
Review Comment:
If we want real fail hard - fail at all incremental rewrites too then we can drop set and put the precondition on manifestinsnapshot against the map.
This way we are 100% sure there is no way we are getting wrong length but we will loose incremental rewrite completely as it fails everytime.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org