liurenjie1024 commented on code in PR #1598:
URL: https://github.com/apache/iceberg-rust/pull/1598#discussion_r2276200813
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -28,32 +28,124 @@ use iceberg::io::{FileIO, FileIOBuilder};
use iceberg::spec::{TableMetadata, TableMetadataBuilder};
use iceberg::table::Table;
use iceberg::{
- Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent,
Result, TableCommit,
- TableCreation, TableIdent,
+ Catalog, CatalogBuilder, Error, ErrorKind, MetadataLocation, Namespace,
NamespaceIdent, Result,
+ TableCommit, TableCreation, TableIdent,
};
use typed_builder::TypedBuilder;
use crate::utils::create_sdk_config;
+/// S3Tables table bucket ARN property
+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 {
+ /// Catalog name.
+ #[builder(default, setter(strip_option))]
+ name: Option<String>,
/// Unlike other buckets, S3Tables bucket is not a physical bucket, but a
virtual bucket
/// that is managed by s3tables. We can't directly access the bucket with
path like
/// s3://{bucket_name}/{file_path}, all the operations are done with
respect of the bucket
/// ARN.
table_bucket_arn: String,
+ /// Endpoint URL for the catalog.
+ #[builder(default)]
+ endpoint_url: Option<String>,
+ /// Optional pre-configured AWS SDK client for S3Tables.
+ #[builder(default)]
+ client: Option<aws_sdk_s3tables::Client>,
/// Properties for the catalog. The available properties are:
/// - `profile_name`: The name of the AWS profile to use.
/// - `region_name`: The AWS region to use.
/// - `aws_access_key_id`: The AWS access key ID to use.
/// - `aws_secret_access_key`: The AWS secret access key to use.
/// - `aws_session_token`: The AWS session token to use.
#[builder(default)]
- properties: HashMap<String, String>,
- /// Endpoint URL for the catalog.
- #[builder(default, setter(strip_option(fallback = endpoint_url_opt)))]
- endpoint_url: Option<String>,
+ props: HashMap<String, String>,
+}
+
+/// Builder for [`S3TablesCatalog`].
+#[derive(Debug)]
+pub struct S3TablesCatalogBuilder(S3TablesCatalogConfig);
+
+/// Default builder for [`S3TablesCatalog`].
+impl Default for S3TablesCatalogBuilder {
+ fn default() -> Self {
+ Self(S3TablesCatalogConfig {
+ name: None,
+ table_bucket_arn: "".to_string(),
+ endpoint_url: None,
+ client: None,
+ props: HashMap::new(),
+ })
+ }
+}
+
+/// 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 {
+ self.0.endpoint_url = Some(endpoint_url.into());
+ self
+ }
+
+ /// Configure the catalog with a pre-built AWS SDK client.
+ pub fn with_client(mut self, client: aws_sdk_s3tables::Client) -> Self {
+ self.0.client = Some(client);
+ self
+ }
+
+ /// Configure the catalog with a table bucket ARN.
+ pub fn with_table_bucket_arn(mut self, table_bucket_arn: impl
Into<String>) -> Self {
+ self.0.table_bucket_arn = table_bucket_arn.into();
+ self
+ }
+}
+
+impl CatalogBuilder for S3TablesCatalogBuilder {
+ type C = S3TablesCatalog;
+
+ fn load(
+ mut self,
+ name: impl Into<String>,
+ props: HashMap<String, String>,
+ ) -> impl Future<Output = Result<Self::C>> + Send {
+ self.0.name = Some(name.into());
+
+ if props.contains_key(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN) {
+ self.0.table_bucket_arn = props
+ .get(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN)
+ .cloned()
+ .unwrap_or_default();
+ }
+
+ if props.contains_key(S3TABLES_CATALOG_PROP_ENDPOINT_URL) {
+ self.0.endpoint_url =
props.get(S3TABLES_CATALOG_PROP_ENDPOINT_URL).cloned();
+ }
+
+ // Collect other remaining properties
+ self.0.props = props
+ .into_iter()
+ .filter(|(k, _)| {
+ k != S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN
+ && k != S3TABLES_CATALOG_PROP_ENDPOINT_URL
+ })
+ .collect();
+
+ async move {
+ if self.0.table_bucket_arn.is_empty() {
Review Comment:
We should check name can't be empty string
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -28,32 +28,124 @@ use iceberg::io::{FileIO, FileIOBuilder};
use iceberg::spec::{TableMetadata, TableMetadataBuilder};
use iceberg::table::Table;
use iceberg::{
- Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent,
Result, TableCommit,
- TableCreation, TableIdent,
+ Catalog, CatalogBuilder, Error, ErrorKind, MetadataLocation, Namespace,
NamespaceIdent, Result,
+ TableCommit, TableCreation, TableIdent,
};
use typed_builder::TypedBuilder;
use crate::utils::create_sdk_config;
+/// S3Tables table bucket ARN property
+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 {
Review Comment:
Make this crate private, and remove the builder.
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -28,32 +28,124 @@ use iceberg::io::{FileIO, FileIOBuilder};
use iceberg::spec::{TableMetadata, TableMetadataBuilder};
use iceberg::table::Table;
use iceberg::{
- Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent,
Result, TableCommit,
- TableCreation, TableIdent,
+ Catalog, CatalogBuilder, Error, ErrorKind, MetadataLocation, Namespace,
NamespaceIdent, Result,
+ TableCommit, TableCreation, TableIdent,
};
use typed_builder::TypedBuilder;
use crate::utils::create_sdk_config;
+/// S3Tables table bucket ARN property
+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 {
+ /// Catalog name.
+ #[builder(default, setter(strip_option))]
+ name: Option<String>,
/// Unlike other buckets, S3Tables bucket is not a physical bucket, but a
virtual bucket
/// that is managed by s3tables. We can't directly access the bucket with
path like
/// s3://{bucket_name}/{file_path}, all the operations are done with
respect of the bucket
/// ARN.
table_bucket_arn: String,
+ /// Endpoint URL for the catalog.
+ #[builder(default)]
+ endpoint_url: Option<String>,
+ /// Optional pre-configured AWS SDK client for S3Tables.
+ #[builder(default)]
+ client: Option<aws_sdk_s3tables::Client>,
/// Properties for the catalog. The available properties are:
/// - `profile_name`: The name of the AWS profile to use.
/// - `region_name`: The AWS region to use.
/// - `aws_access_key_id`: The AWS access key ID to use.
/// - `aws_secret_access_key`: The AWS secret access key to use.
/// - `aws_session_token`: The AWS session token to use.
#[builder(default)]
- properties: HashMap<String, String>,
- /// Endpoint URL for the catalog.
- #[builder(default, setter(strip_option(fallback = endpoint_url_opt)))]
- endpoint_url: Option<String>,
+ props: HashMap<String, String>,
+}
+
+/// Builder for [`S3TablesCatalog`].
+#[derive(Debug)]
+pub struct S3TablesCatalogBuilder(S3TablesCatalogConfig);
+
+/// Default builder for [`S3TablesCatalog`].
+impl Default for S3TablesCatalogBuilder {
+ fn default() -> Self {
+ Self(S3TablesCatalogConfig {
+ name: None,
+ table_bucket_arn: "".to_string(),
+ endpoint_url: None,
+ client: None,
+ props: HashMap::new(),
+ })
+ }
+}
+
+/// 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 {
Review 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.
##########
Cargo.toml:
##########
@@ -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"
Review Comment:
Why do we need to update these?
##########
crates/catalog/s3tables/src/catalog.rs:
##########
@@ -28,32 +28,124 @@ use iceberg::io::{FileIO, FileIOBuilder};
use iceberg::spec::{TableMetadata, TableMetadataBuilder};
use iceberg::table::Table;
use iceberg::{
- Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent,
Result, TableCommit,
- TableCreation, TableIdent,
+ Catalog, CatalogBuilder, Error, ErrorKind, MetadataLocation, Namespace,
NamespaceIdent, Result,
+ TableCommit, TableCreation, TableIdent,
};
use typed_builder::TypedBuilder;
use crate::utils::create_sdk_config;
+/// S3Tables table bucket ARN property
+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 {
+ /// Catalog name.
+ #[builder(default, setter(strip_option))]
+ name: Option<String>,
/// Unlike other buckets, S3Tables bucket is not a physical bucket, but a
virtual bucket
/// that is managed by s3tables. We can't directly access the bucket with
path like
/// s3://{bucket_name}/{file_path}, all the operations are done with
respect of the bucket
/// ARN.
table_bucket_arn: String,
+ /// Endpoint URL for the catalog.
+ #[builder(default)]
+ endpoint_url: Option<String>,
+ /// Optional pre-configured AWS SDK client for S3Tables.
+ #[builder(default)]
+ client: Option<aws_sdk_s3tables::Client>,
/// Properties for the catalog. The available properties are:
/// - `profile_name`: The name of the AWS profile to use.
/// - `region_name`: The AWS region to use.
/// - `aws_access_key_id`: The AWS access key ID to use.
/// - `aws_secret_access_key`: The AWS secret access key to use.
/// - `aws_session_token`: The AWS session token to use.
#[builder(default)]
- properties: HashMap<String, String>,
- /// Endpoint URL for the catalog.
- #[builder(default, setter(strip_option(fallback = endpoint_url_opt)))]
- endpoint_url: Option<String>,
+ props: HashMap<String, String>,
+}
+
+/// Builder for [`S3TablesCatalog`].
+#[derive(Debug)]
+pub struct S3TablesCatalogBuilder(S3TablesCatalogConfig);
+
+/// Default builder for [`S3TablesCatalog`].
+impl Default for S3TablesCatalogBuilder {
+ fn default() -> Self {
+ Self(S3TablesCatalogConfig {
+ name: None,
+ table_bucket_arn: "".to_string(),
+ endpoint_url: None,
+ client: None,
+ props: HashMap::new(),
+ })
+ }
+}
+
+/// 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 {
+ self.0.endpoint_url = Some(endpoint_url.into());
+ self
+ }
+
+ /// Configure the catalog with a pre-built AWS SDK client.
+ pub fn with_client(mut self, client: aws_sdk_s3tables::Client) -> Self {
+ self.0.client = Some(client);
+ self
+ }
+
+ /// Configure the catalog with a table bucket ARN.
+ pub fn with_table_bucket_arn(mut self, table_bucket_arn: impl
Into<String>) -> Self {
Review Comment:
Similar to above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
