Skip to content
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

Merged
merged 10 commits into from
Nov 24, 2023

Conversation

liurenjie1024
Copy link
Contributor

Close #60

Last part of first version of rest catalog, add create table api and update table api.

Also introduces transaction api.

@liurenjie1024 liurenjie1024 marked this pull request as draft November 16, 2023 08:45
Copy link
Member

@Xuanwo Xuanwo left a 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.

@liurenjie1024 liurenjie1024 marked this pull request as ready for review November 20, 2023 03:55
@liurenjie1024
Copy link
Contributor Author

This is already ready for review. cc @Fokko @Xuanwo @ZENOTME @JanKaul

Copy link
Contributor

@Fokko Fokko left a 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!

crates/catalog/rest/src/catalog.rs Outdated Show resolved Hide resolved
crates/catalog/rest/src/catalog.rs Outdated Show resolved Hide resolved
("total-position-deletes", "0"),
("total-equality-deletes", "0")
].iter().map(|p|(p.0.to_string(), p.1.to_string())))
("spark.app.id", "local-1646787004168"),
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@liurenjie1024 liurenjie1024 Nov 27, 2023

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.

crates/catalog/rest/src/catalog.rs Show resolved Hide resolved
crates/iceberg/src/spec/table_metadata.rs Outdated Show resolved Hide resolved
@liurenjie1024
Copy link
Contributor Author

liurenjie1024 commented Nov 21, 2023

As I mentioned earlier, having the REST Docker images would make testing a lot easier.

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.

@liurenjie1024
Copy link
Contributor Author

cc @Fokko I've fixed comment, PTAL

Copy link
Contributor

@ZENOTME ZENOTME left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@liurenjie1024
Copy link
Contributor Author

cc @Xuanwo Any other comments?

@Xuanwo
Copy link
Member

Xuanwo commented Nov 24, 2023

cc @Xuanwo Any other comments?

LGTM, let's go!

Copy link
Contributor

@Fokko Fokko left a 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!

Thanks @Xuanwo and @ZENOTME for the review!

@Fokko Fokko merged commit a6d98c3 into apache:main Nov 24, 2023
6 checks passed
@Fokko Fokko mentioned this pull request Nov 18, 2024
28 tasks
shaeqahmed pushed a commit to matanolabs/iceberg-rust that referenced this pull request Dec 9, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement rest catalog.
4 participants