[1.10.x] Core, Spark: Ensure correct delete file sizes in rewrite table action - #17215
Conversation
Backport of apache#15470 to 1.10.x. Rewriting a position delete file's path references changes its on-disk size, but the rewritten manifest kept the original file_size_in_bytes. Readers that trust file_size_in_bytes to elide a stat() call can fail. Measure the rewritten position delete file from the writer after close and thread that size through rewriteDeleteManifest so the manifest entry records the true size. Entries that were not rewritten keep their original size. The DV/Puffin handling from the original change is omitted since 1.10.x does not have deletion-vector rewrite support.
|
cc @mbutrovich this is a backport of your PR to 1.10, but there are some differences outlined in the PR description. |
|
Thanks for handling the backport @amogh-jahagirdar! I will review this today. |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! validated that the core logic is there
there are a few missing diffs from the original PR, but none of those are critical for backport
- javadoc for
rebuildMetadata
- * <li>Rebuild manifest to staging
+ * <li>Rewrite referenced position delete files to staging
+ * <li>Rebuild manifests to staging
- delete-entry fallback test in
core/src/test/java/org/apache/iceberg/TestRewriteTablePathUtil.java(testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries)
For a backport PR, i think its fine to omit them
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
I think the test testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries would be nice to have, but I'm still getting up to speed on what backport workflows look like on the Iceberg Java repo. Otherwise LGTM! Thanks @amogh-jahagirdar!
nssalian
left a comment
nssalian
left a comment
There was a problem hiding this comment.
lgtm for the backport. But worth a quick follow-up to port the dropped testRewriteDeleteManifestFallsBackToOriginalSizeForDeletedEntries core test and the rebuildMetadata javadoc update
amogh-jahagirdar
changed the title
|
Thanks for addressing the test and javadoc in the latest commit, @amogh-jahagirdar! |
|
Thanks all! |
4 participants
Backport of #15470 to 1.10.x.
This backport has the following differences with the original:
1.) The DV/Puffin handling from the original change is omitted since 1.10.x does not DV support in rewrite table procedure. Adding this DV support in the rewrite procedure is basically a feature of its own and wouldn't be appropriate for a backport.
2.) Different spark versions that we supported in 1.10. This one is 3.4/3.5/4.0.
3.) Core library APIs for rewrite table procedure that were deprecated in master, are ported without deprecation markers since it's a backport anyways.