Uh oh!
There was an error while loading. Please reload this page.
Core: Disallow adding and removing the same file in a rewrite files commit - #17359
Core: Disallow adding and removing the same file in a rewrite files commit#17359amogh-jahagirdar wants to merge 3 commits into
Conversation
Uh oh!
There was an error while loading. Please reload this page.
| private static final RateLimiterStrategy LOW_RATE = RateLimiterStrategy.perSecond(1.0 / 10000.0); | ||
| @TempDir private File checkpointDir; | ||
| @TempDir private java.nio.file.Path dataDir; |
There was a problem hiding this comment.
not needed, use table.locationProvider().newDataLocation..
| for (DataFile added : addedDataFiles()) { | ||
| Preconditions.checkArgument( | ||
| !replacedDataFiles.contains(added), | ||
| "Cannot add and delete the same file in the same rewrite: %s", | ||
| added.location()); | ||
| } |
There was a problem hiding this comment.
I am wondering if this is the right place to place the guard, if the goal is to prevent the same file being added and removed in the same snapshot? I think both overwrite-files and row-delta can be abused in a way to achieve the same goal.
On a separate note, knowing this produce incorrect result for the change tracking detection, sometimes such surgical operation can be helpful for stats backfill as to rectify a previously written file with incorrect column level stats, want to see if we shall encourage to use dedicated repair actions such as proposed in #10784
There was a problem hiding this comment.
Yeah I was looking at this in the interim, I think you're right that Overwrite and Row Delta technically allow this and we should block them too.
On a separate note, knowing this produce incorrect result for the change tracking detection, sometimes such surgical operation can be helpful for stats backfill as to rectify a previously written file with incorrect column level stats, want to see if we shall encourage to use dedicated repair actions such as proposed in #10784
Yup! I think rewriting manifests (just like Repair would do) is the right way. After some thought, I don't think updating these operations to infer that add + remove = existing is the right thing to do because I think these APIs have largely been designed around the fact that added = "something new", and removed = "something that existed and we need to remove".
| } | ||
| @TestTemplate | ||
| public void addingAndDeletingSameFileDisallowed() { |
There was a problem hiding this comment.
I think we might also need to fix a few other tests, namely testReadStreamWithSnapshotTypeRewriteDataFilesIgnoresReplaceMaxFilesAndRows because of
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 pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Relates to #17198
Currently rewrite files allows us to add and remove the same file in a commit. Arguably this is incorrect, as it's an issue with change detection (whether the file was truly added or removed in that commit is ambiguous). It also can lead to ambiguity in file cleanup (though not in the reference implementation's file cleanup since it does a full a reachability analysis).