-
Notifications
You must be signed in to change notification settings - Fork 363
removed references of BEFORE/AFTER_COMMIT_VIEW #3554
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
snazy
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.
The change LGTM!
In the future, please just rebase the existing PR (#3441) instead of opening a new one.
| AFTER_REPLACE_VIEW, | ||
| BEFORE_COMMIT_VIEW, | ||
| AFTER_COMMIT_VIEW, | ||
| BEFORE_COMMIT_VIEW, // REMOVED FROM SOURCE CODE |
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'm catching up here and saw this related conversation:
https://github.com/apache/polaris/pull/3195/changes#r2593440033
I'm not sure I agree with keeping these constants. Granted the ordinals will change if we remove them. But the feature is in beta status right now. It feels odd to see deprecated/unused constants in an API that is not even "official" yet.
@adnanhemani @dimas-b what do you think?
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 do not see any particular issue with changing ordinals. AFAIK, Polaris does not use java serialization for this enum and Jackson deals with names.
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.
If we are to remove the constants, will be ok doing in a follow up PR?cc @adutra I can take ownership of that issue as well.
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 think doing it in a separate PR makes sense, so the two other similar enum values can be removed as well.
This PR addresses issue #3418 after this PR recently did the same for tables.
Given that the current implementation of commitTransaction works only for table (from my reading, please correct if I am wrong), I concluded only removing events would be enough especially since *ReplaceView is already supported.
Tests:
Ran integration tests.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)