amogh-jahagirdar commented on code in PR #8520:
URL: https://github.com/apache/iceberg/pull/8520#discussion_r1330824697
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -421,6 +421,7 @@ private void commitSimpleTransaction() {
.onlyRetryOn(CommitFailedException.class)
.run(
underlyingOps -> {
+ deletedFiles.clear();
Review Comment:
Yes you're right, I stepped through the code a bit more, really the
applyUpdates is more of a `reapplyUpdates` (in case the table state changed).
`applyUpdates` is a no-op on the first attempt because the individual
operations commit has already completed. If the deletedFiles set is cleared
before the first commit and that commit succeeds we would never end up cleaning
those files based on the cleanup done here
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L450.
Although I don't know why our tests wouldn't pick up on this, we have tests
which validate we cleanup the files we would expect
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
