chore: Improve clarity when using CatalogConfigs in CatalogBuilder
#1873
Conversation
jonathanc-n
left a comment
jonathanc-n
left a comment
There was a problem hiding this comment.
Just some comments to clarify to reviewers
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @jonathanc-n for this pr, I have some suggestions to improve the readability.
crates/catalog/glue/src/catalog.rs
Outdated
|
|
||
| impl GlueCatalogBuilder { | ||
| /// Get a mutable reference to the catalog configuration. | ||
| pub(crate) fn catalog_config(&mut self) -> &mut GlueCatalogConfig { |
There was a problem hiding this comment.
I'm not a big fan of such a change. I think a better approach would be following:
pub struct GlueCatalogBuilder {
name: Option<String>,
....
}
struct GlueCatalogConfig {
name: String
}This makes things easier to read, more importantly, the config class type safe. Builder struct need to store intermediate states, so it's reasonable to have it contains many optional fields. The config struct, which is finally built and verfied, should only contain valid states, e.g. it should no longer has unnecessary optional fields.
| pub(crate) struct GlueCatalogConfig { | ||
| name: Option<String>, | ||
| #[allow(dead_code)] // can be used for debugging | ||
| name: String, |
There was a problem hiding this comment.
nit: It's a little annoying to have this #[allow(dead_code)] attribute here, is it possible to figure out a way to remove this? For example, adding a Display method, etc.
crates/catalog/hms/src/catalog.rs
Outdated
| #[derive(Debug)] | ||
| pub(crate) struct HmsCatalogConfig { | ||
| #[allow(dead_code)] // Stored for debugging and potential future use | ||
| name: Option<String>, |
There was a problem hiding this comment.
This is required, we should remove the Option
| pub(crate) struct RestCatalogConfig { | ||
| #[allow(dead_code)] // Stored for debugging and potential future use | ||
| #[builder(default, setter(strip_option))] | ||
| name: Option<String>, |
| } | ||
|
|
||
| /// Rest catalog configuration. | ||
| #[derive(Clone, Debug, TypedBuilder)] |
There was a problem hiding this comment.
Do we still need this TypedBuilder?
| pub(crate) struct S3TablesCatalogConfig { | ||
| /// Catalog name. | ||
| #[allow(dead_code)] // Stored for debugging and potential future use | ||
| name: Option<String>, |
| uri: "".to_string(), | ||
| name: "".to_string(), | ||
| warehouse_location: "".to_string(), | ||
| Self { |
There was a problem hiding this comment.
I think we could also derive Default?
| } | ||
| } | ||
|
|
||
| impl MemoryCatalogBuilder { |
2 participants
Which issue does this PR close?
CatalogBuilder.0more explicit #1862 .What changes are included in this PR?
Makes
CatalogConfigcalls more explicit.Are these changes tested?