-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix "AlreadyExistsException: Location already exists" at rewrite_table_path #14859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…s/TestRewriteTablePathsAction.java
...k/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java
Outdated
Show resolved
Hide resolved
|
@nastra can you please review the changes |
...k/v4.0/spark/src/main/java/org/apache/iceberg/spark/actions/RewriteTablePathSparkAction.java
Show resolved
Hide resolved
|
@dramaticlly fixed the review comments, can you review it now. |
|
@Bhargavkonidena it would be nice to have a test for #14814 if possible |
4dfda1b to
c86dc63
Compare
c86dc63 to
5ff0f4a
Compare
5ff0f4a to
053c664
Compare
Done |
|
@nastra can you please review it now? |
| // rebuild position delete files | ||
| // Use DeleteFileSet to ensure proper equality comparison based on file location, content | ||
| // offset, | ||
| // and content size. This is particularly important for deletion vectors (DV files) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this line can go with the previous one
| * based on file location, content offset, and content size. | ||
| */ | ||
| @TestTemplate | ||
| public void testPositionDeletesWithMultipleTagsOnSameSnapshot() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this test actually reproduces the issue because the test passes for me even without the fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nastra Updated the test case, can you validate it now.
Fix #14814