-
Notifications
You must be signed in to change notification settings - Fork 189
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: Implement create table and update table api for rest catalog. #97
Conversation
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, just need to make CI happy.
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.
Some small comments, and there is a lot in here. As I mentioned earlier, having the REST Docker images would make testing a lot easier. Apart from that it, looks good!
("total-position-deletes", "0"), | ||
("total-equality-deletes", "0") | ||
].iter().map(|p|(p.0.to_string(), p.1.to_string()))) | ||
("spark.app.id", "local-1646787004168"), |
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: shouldn't this be caught by the linter? Same for the trailing comma's below.
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.
Sorry, I don't quite get your point.
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.
Why is this changed? It looks only indentation has changed, and most linters would not allow that since it makes the linter non-deterministic.
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.
Seems rust-fmt not enforcing the indentation, let me do some check.
I've tracked this in #100, and will do it after this get merged. This pr is already too large for me to add more stuff. |
cc @Fokko I've fixed comment, PTAL |
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!
cc @Xuanwo Any other comments? |
LGTM, let's go! |
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.
Looking great @liurenjie1024 Thanks for working on this!
…pache#97) * feat: Implement update table/create table api for rest catalog * Add create table test * Add some tests for update table * Add tests for table updates and requirements * Add some comments * Add tests for transaction * Fix format * Fix linters * Stop upgrading to uuid 1.6.0 * Fix comments
Close #60
Last part of first version of rest catalog, add create table api and update table api.
Also introduces transaction api.