feat(catalog): Add catalog loader and builder implementation for rest catalog#1580
Conversation
| use iceberg_catalog_rest::RestCatalogBuilder; | ||
|
|
||
| #[async_trait] | ||
| pub trait BoxedCatalogBuilder { |
There was a problem hiding this comment.
I think we can just put this in builder.rs under /crates/catalog/rest/src/
There was a problem hiding this comment.
Thanks for taking a look!
I'm not sure if it's necessary because I think BoxedCatalogBuilder is suppose to be generic to use for all catalog builder implementations. Like in L42, we simply call the load method in the trait to return the corresponding catalog and can be extended to add more catalogs in load, so it's not really rest catalog specific.
There was a problem hiding this comment.
Sorry I meant the common code like this can be put under crates/iceberg/src/catalog/mod.rs. I don't see the need of adding another module here, maybe I'm missing anything?
Also I'm thinking if Box<Self> will be common across all catalog builders, maybe we can just change the CatalogBuilder trait to be self: Arc<Self> or self: Box<Self>, so we don't need to define a wrapper trait like this, wdyt?
There was a problem hiding this comment.
For putting these in crates/iceberg/src/catalog/mod.rs, I think it's fine, but if we want to add it as dependency to playground or integration test framework, does it mean we need to import the catalog crate individually?
For the Box<Self>, I think we are only consuming the CatalogBuilder and doesn't need to share it, so I think Box<Self> is fine. cc: @liurenjie1024 wondering if you have any suggestion on this?
There was a problem hiding this comment.
For the Box, I think we are only consuming the CatalogBuilder and doesn't need to share it, so I think Box is fine.
+1. I think it's fine to put the BoxedCatalogBuilder here since it's mainly used by load function.
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @lliangyu-lin for this pr, generally LGTM! Just missing some doc and a little change.
| use iceberg_catalog_rest::RestCatalogBuilder; | ||
|
|
||
| #[async_trait] | ||
| pub trait BoxedCatalogBuilder { |
There was a problem hiding this comment.
For the Box, I think we are only consuming the CatalogBuilder and doesn't need to share it, so I think Box is fine.
+1. I think it's fine to put the BoxedCatalogBuilder here since it's mainly used by load function.
crates/catalog/rest/src/catalog.rs
Outdated
|
|
||
| /// Rest catalog configuration. | ||
| #[derive(Clone, Debug, TypedBuilder)] | ||
| pub struct RestCatalogConfig { |
There was a problem hiding this comment.
We should mark this as crate private, and adding doc to explain how to load a RestCatalog with the new approach, e.g. using the RestCatalogBuilder. Otherwise user will be confusing since we will have two methods for building rest catalog.
There was a problem hiding this comment.
I made the RestCatalogConfig crate private, seems like it's backward incompatible change as previous catalog new will require a RestCatalogConfig.
crates/catalog/rest/src/catalog.rs
Outdated
| RegisterTableRequest, RenameTableRequest, | ||
| }; | ||
|
|
||
| const REST_CATALOG_PROP_URI: &str = "uri"; |
There was a problem hiding this comment.
We should make them public as users are epxecting to use these constants rather than raw string.
crates/catalog/rest/src/catalog.rs
Outdated
| /// Builder for [`RestCatalog`]. | ||
| /// To build a rest catalog with configurations | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust, no_run | ||
| /// use std::collections::HashMap; | ||
| /// | ||
| /// use iceberg::CatalogBuilder; | ||
| /// use iceberg_catalog_rest::RestCatalogBuilder; | ||
| /// | ||
| /// #[tokio::main] | ||
| /// async fn main() { | ||
| /// let catalog = RestCatalogBuilder::default() | ||
| /// .load( | ||
| /// "rest", | ||
| /// HashMap::from([ | ||
| /// ("uri".to_string(), "http://localhost:8181".to_string()), | ||
| /// ("warehouse".to_string(), "s3://warehouse".to_string()), | ||
| /// ]), | ||
| /// ) | ||
| /// .await | ||
| /// .unwrap(); | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
How about moving this crate's doc, e.g. in lib.rs? This should be the first thing a user sees when using this crate.
There was a problem hiding this comment.
Yep, I think moving to lib.rs make sense.
crates/catalog/rest/src/catalog.rs
Outdated
| /// .load( | ||
| /// "rest", | ||
| /// HashMap::from([ | ||
| /// ("uri".to_string(), "http://localhost:8181".to_string()), |
There was a problem hiding this comment.
We should use constants rather than raw string as better practice. Please change all occurences.
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @lliangyu-lin for this pr!
|
It seems this PR didn't include the |
## Which issue does this PR close? - None ## What changes are included in this PR? - Auto-update `Cargo.lock` based on the changes from #1580 ## Are these changes tested? Using the existing uts
… catalog (apache#1580) ## Which issue does this PR close? - Part of apache#1260 - Closes apache#1255 ## What changes are included in this PR? * Some of the implementations are directly coming from the design in apache#1231 * Added a new crate `loader` for loading catalog based on type. * Implement the CatalogBuilder trait for rest catalog ## Are these changes tested? Yes, by unit tests --------- Co-authored-by: liurenjie1024 <liurenjie2008@gmail.com>
## Which issue does this PR close? - None ## What changes are included in this PR? - Auto-update `Cargo.lock` based on the changes from apache#1580 ## Are these changes tested? Using the existing uts
|
Hi, this change (particularly removing |
The motivation behind the refactoring is to have an unified way to load catalogs. We had some discussions about type safe configs, but we need to a design for it. The old |
4 participants
Which issue does this PR close?
What changes are included in this PR?
loaderfor loading catalog based on type.Are these changes tested?
Yes, by unit tests