-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Core: Cleaning up uncommitted cleanup logic in SnapshotProducer implementations #14923
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
Conversation
dramaticlly
left a comment
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.
Cleanup LGTM but there's opportunity for a bit more
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
Outdated
Show resolved
Hide resolved
9a4aca5 to
c143096
Compare
c143096 to
3205ee3
Compare
3205ee3 to
804fde5
Compare
geruh
left a comment
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.
This looks good to me!
singhpk234
left a comment
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.
LGTM, thanks @amogh-jahagirdar !
|
Thanks @amogh-jahagirdar for the PR! Thanks @dramaticlly @geruh @singhpk234 for the review! |
Was going through MergingSnapshotProducer code and noticed that there's a bit of redundancy in the cleanUncommittedAppends logic, so did a refactoring there. Also cleaned up the preconditions check for format 3 and 4 for delete files, since it's the same condition, we can collapse those cases.