-
Notifications
You must be signed in to change notification settings - Fork 276
Experiment implementation for catalog builder #1231
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
base: main
Are you sure you want to change the base?
Conversation
I also want to remove builder for catalog configs. |
cc @Xuanwo I've figured out a way to keep both traits without introducing an enum, see
WDYT? |
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.
@liurenjie1024 thanks for the initiative!
For my understanding I first need to answer this question:
Are we sure that we need the CatalogBuilder
trait? What would be its purpose?
My initial idea would be to just have different typesafe builders for different catalogs. The current CatalogBuilder
trait currently contains fields that are not required for some catalogs. For example in-memory
or dynamo
don't even need a uri.
On the other hand we have the rest catalog that needs significantly more configurations, and I don't think we want to make those key value pairs. I would be in favor to have the interfaces as typesafe as possible.
|
||
impl RestCatalogBuilder { | ||
/// Configures the catalog with a custom HTTP client. | ||
pub fn with_client(&mut self, client: Client) -> &mut Self { |
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.
cc @Xuanwo
Hi, @c-thiel Sorry for misclarification. For more background, please refer to #1228. In short, we are trying to develop a catalog loader so that it can be used by some applications such as iceberg-playground or data driven integration test framework.
I've refined the trait definition as proposed by @Xuanwo in #1372 .
I think a pure type safe approach may not be practical since we also need to pass file io's configurations through catalog, and the actual file io used is determined at runtime. As a compromise, we could define a config struct for each catalog like following:
WDYT? |
Makes sense to me |
What changes are included in this PR?
Demo implementation for catalog builder.
Are these changes tested?
UT.