-
Notifications
You must be signed in to change notification settings - Fork 343
feat: Introduce snapshot summary properties #1336
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
feat: Introduce snapshot summary properties #1336
Conversation
jonathanc-n
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.
Looks good to me, sorry for the misunderstanding. Thanks for this pr @dentiny!
d6d554b to
e268be7
Compare
Xuanwo
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.
Nice change, thank you!
|
One thing left here is to format the code. |
|
Hi @Xuanwo , I think the linting issue has been fixed, also merged with main branch; could you please take another look when you have some time? Thank you! |
|
@Xuanwo friendly ping :) |
Xuanwo
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.
Nice change, thank you @dentiny!
| }, | ||
| /// Add snapshot summary properties. | ||
| #[serde(rename_all = "kebab-case")] | ||
| AddSnapshotSummaryProperties { |
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 table update doesn't exist in table updates, see https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml
|
Hi, @Xuanwo @dentiny We need to revert this pr, the new added update doesn't exist in openapi spec: https://github.com/apache/iceberg/blob/cab0decbb0e32bf314039e30807eb033c50665d5/open-api/rest-catalog-open-api.yaml#L3058 |
Hi @liurenjie1024 , there's a |
|
Also I don't think we should allow user to udpate snapshot summaries alone, it's dangerous. |
HI @liurenjie1024, curious if you have other suggestions to update snapshot summary? The Java impl I linked above seems to fulfill the requirement, and is exposed as public? |
The |
This reverts commit 8bb9a88.
Yeah I actually checked existing interface before I made this PR, I didn't any interface exposing the capability to set / update snapshot summary: iceberg-rust/crates/iceberg/src/transaction/append.rs Lines 40 to 58 in 3ea6f47
|
## Which issue does this PR close? - Closes #1329 ## What changes are included in this PR? This PR tries to do the same thing as reverted [PR](#1336), which adds capability to set snapshot summary properties. Followup on thread: #1336 (comment), in this PR I took a different way, which expose interfaces within fast append action. ## Are these changes tested? Yes, I add new unit tests.
I think you find the correct way to do it in #1391 |
Which issue does this PR close?
TLDR:
What changes are included in this PR?
I add a new table update action for snapshot summary properties.
Are these changes tested?
Yes, unit tests are added.