Avoid panic when deleting more files than adding in an overwrite#2
Open
timsaucer wants to merge 1 commit intoglitchy:feat/overwrite-actionfrom
Open
Conversation
timsaucer
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is targeting the branch for PR apache#2185
I was testing the PR above and found an issue when I was overwriting with fewer files than I was deleting. If you do that it causes a panic in
update_snapshot_summariesbecause we end up subtracting an unsigned integer and going negative.We could patch that to avoid the underflow, but the root cause is that in
summary()we never get the previous snapshot - it's always None. This is because the snapshot producer hasn't run yet so the new snapshot ID can never be retrieved from the table metadata. Instead we can get the current snapshot of the table and use that directly.I have updated the unit test to emphasize this by registering two files and then overwriting with deleting both and adding one. If you revert the change in
summary()you'll see the test panic.