feat(catalog): implement catalog loader for s3tables #1598
Conversation
fvaleye
force-pushed
the
feature/implement-catalog-for-s3table
branch
4 times, most recently
from
8df8266 to
a39a467
Compare
fvaleye
changed the title
- Add supported_types() to list registry entries - Include supported types in unsupported-type error
fvaleye
force-pushed
the
feature/implement-catalog-for-s3table
branch
7 times, most recently
from
19b9534 to
648a0be
Compare
- Update hive_metastore dependency from 0.1 to 0.2.0 - Fixes derivative attribute compilation errors after rebase - Ensures compatibility with updated dependency tree
fvaleye
force-pushed
the
feature/implement-catalog-for-s3table
branch
from
648a0be to
4123e4d
Compare
There was a problem hiding this comment.
Thanks @fvaleye for this pr, generally looks good! Left some comments.
| pub const S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN: &str = "table_bucket_arn"; | ||
| /// S3Tables endpoint URL property | ||
| pub const S3TABLES_CATALOG_PROP_ENDPOINT_URL: &str = "endpoint_url"; | ||
|
|
||
| /// S3Tables catalog configuration. | ||
| #[derive(Debug, TypedBuilder)] | ||
| pub struct S3TablesCatalogConfig { |
There was a problem hiding this comment.
Make this crate private, and remove the builder.
There was a problem hiding this comment.
I will make the change.
| .collect(); | ||
|
|
||
| async move { | ||
| if self.0.table_bucket_arn.is_empty() { |
There was a problem hiding this comment.
We should check name can't be empty string
There was a problem hiding this comment.
That's right, I'll add a dedicated test condition.
| /// Builder methods for [`S3TablesCatalog`]. | ||
| impl S3TablesCatalogBuilder { | ||
| /// Configure the catalog with a custom endpoint URL (useful for local testing/mocking). | ||
| pub fn with_endpoint_url(mut self, endpoint_url: impl Into<String>) -> Self { |
There was a problem hiding this comment.
This method allows user to config url from either prop or this method. I'm not totally against this design, but please add doc to explain the behavior when both appears, also please add tests for it.
There was a problem hiding this comment.
Ok, I'll add more documentation and proper unit tests.
| } | ||
|
|
||
| /// Configure the catalog with a table bucket ARN. | ||
| pub fn with_table_bucket_arn(mut self, table_bucket_arn: impl Into<String>) -> Self { |
| @@ -73,10 +74,11 @@ faststr = "0.2.31" | |||
| fnv = "1.0.7" | |||
| fs-err = "3.1.0" | |||
| futures = "0.3" | |||
| hive_metastore = "0.1" | |||
| hive_metastore = "0.2.0" | |||
There was a problem hiding this comment.
Why do we need to update these?
There was a problem hiding this comment.
The hive_metastore was upgraded from 0.1.0 to address an issue after adding S3 tables in the workspace because the HMS catalog and S3Tables catalog share common Rust dependencies (pilota, volo-thrift, etc).
These shared dependencies created a version with numerous conflicts that manifested as "derivative attribute compilation errors".
Just one example among many others:
error: cannot find attribute `derivative` in this scope
--> /Users/florian.valeye/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/hive_metastore-0.1.0/src/hms.rs:201644:15
|
201644 | #[derivative(Default)]
Upgrading to hive_metastore 0.2.0 resolved these transitive dependency conflicts.
…tion - Add comprehensive documentation for with_endpoint_url and with_table_bucket_arn methods explaining property precedence behavior - Add validation to prevent empty or whitespace-only catalog names - Remove unused typed-builder dependency and refactor to manual struct construction
Which issue does this PR close?
What changes are included in this PR?
Catalog Registry Refactor
CATALOG_REGISTRYmapping catalog type strings (e.g.,"rest","s3tables") to their corresponding builder factory closures.load()and centralizes catalog registration logic.New S3 Tables Catalog Support
"s3tables"catalog type to the registry incrates/catalog/loader.iceberg-catalog-s3tablescrate.Hive metastore
hive_metastorewas upgraded from0.1.0to fix after rebasing your S3Tables feature branch because the HMS catalog and S3Tables catalog share common Rust dependencies (pilota, volo-thrift, etc.)Upgrading to hive_metastore
0.2.0resolved these transitive dependency conflicts.Are these changes tested?
Yes.
Added new unit tests for the S3 Tables catalog.
Expanded loader tests to:
CatalogLoaderAPI for both"rest"and"s3tables".