From 3071456b1d7e8f0a4f50315a417290a49f74e0f3 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Wed, 19 Nov 2025 00:46:15 -0500 Subject: [PATCH 1/6] chore: Improve clarity when using `CatalogConfig`s in `CatalogBuilder` --- crates/catalog/glue/src/catalog.rs | 29 ++++++++++---- crates/catalog/hms/src/catalog.rs | 31 ++++++++++----- crates/catalog/rest/src/catalog.rs | 41 +++++++++++++------- crates/catalog/s3tables/src/catalog.rs | 31 ++++++++++----- crates/catalog/sql/src/catalog.rs | 39 ++++++++++++------- crates/iceberg/src/catalog/memory/catalog.rs | 25 +++++++++--- 6 files changed, 134 insertions(+), 62 deletions(-) diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs index 4514f2d7ab..b33bc39a7b 100644 --- a/crates/catalog/glue/src/catalog.rs +++ b/crates/catalog/glue/src/catalog.rs @@ -65,6 +65,18 @@ impl Default for GlueCatalogBuilder { } } +impl GlueCatalogBuilder { + /// Get a mutable reference to the catalog configuration. + pub(crate) fn catalog_config(&mut self) -> &mut GlueCatalogConfig { + &mut self.0 + } + + /// Consume the builder and return the catalog configuration. + pub(crate) fn into_config(self) -> GlueCatalogConfig { + self.0 + } +} + impl CatalogBuilder for GlueCatalogBuilder { type C = GlueCatalog; @@ -73,25 +85,25 @@ impl CatalogBuilder for GlueCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.catalog_config().name = Some(name.into()); if props.contains_key(GLUE_CATALOG_PROP_URI) { - self.0.uri = props.get(GLUE_CATALOG_PROP_URI).cloned() + self.catalog_config().uri = props.get(GLUE_CATALOG_PROP_URI).cloned() } if props.contains_key(GLUE_CATALOG_PROP_CATALOG_ID) { - self.0.catalog_id = props.get(GLUE_CATALOG_PROP_CATALOG_ID).cloned() + self.catalog_config().catalog_id = props.get(GLUE_CATALOG_PROP_CATALOG_ID).cloned() } if props.contains_key(GLUE_CATALOG_PROP_WAREHOUSE) { - self.0.warehouse = props + self.catalog_config().warehouse = props .get(GLUE_CATALOG_PROP_WAREHOUSE) .cloned() .unwrap_or_default(); } // Collect other remaining properties - self.0.props = props + self.catalog_config().props = props .into_iter() .filter(|(k, _)| { k != GLUE_CATALOG_PROP_URI @@ -100,21 +112,22 @@ impl CatalogBuilder for GlueCatalogBuilder { }) .collect(); + let config = self.into_config(); async move { - if self.0.name.is_none() { + if config.name.is_none() { return Err(Error::new( ErrorKind::DataInvalid, "Catalog name is required", )); } - if self.0.warehouse.is_empty() { + if config.warehouse.is_empty() { return Err(Error::new( ErrorKind::DataInvalid, "Catalog warehouse is required", )); } - GlueCatalog::new(self.0).await + GlueCatalog::new(config).await } } } diff --git a/crates/catalog/hms/src/catalog.rs b/crates/catalog/hms/src/catalog.rs index b7d192210b..ff57921727 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -66,6 +66,18 @@ impl Default for HmsCatalogBuilder { } } +impl HmsCatalogBuilder { + /// Get a mutable reference to the catalog configuration. + pub(crate) fn catalog_config(&mut self) -> &mut HmsCatalogConfig { + &mut self.0 + } + + /// Consume the builder and return the catalog configuration. + pub(crate) fn into_config(self) -> HmsCatalogConfig { + self.0 + } +} + impl CatalogBuilder for HmsCatalogBuilder { type C = HmsCatalog; @@ -74,14 +86,14 @@ impl CatalogBuilder for HmsCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.catalog_config().name = Some(name.into()); if props.contains_key(HMS_CATALOG_PROP_URI) { - self.0.address = props.get(HMS_CATALOG_PROP_URI).cloned().unwrap_or_default(); + self.catalog_config().address = props.get(HMS_CATALOG_PROP_URI).cloned().unwrap_or_default(); } if let Some(tt) = props.get(HMS_CATALOG_PROP_THRIFT_TRANSPORT) { - self.0.thrift_transport = match tt.to_lowercase().as_str() { + self.catalog_config().thrift_transport = match tt.to_lowercase().as_str() { THRIFT_TRANSPORT_FRAMED => HmsThriftTransport::Framed, THRIFT_TRANSPORT_BUFFERED => HmsThriftTransport::Buffered, _ => HmsThriftTransport::default(), @@ -89,13 +101,13 @@ impl CatalogBuilder for HmsCatalogBuilder { } if props.contains_key(HMS_CATALOG_PROP_WAREHOUSE) { - self.0.warehouse = props + self.catalog_config().warehouse = props .get(HMS_CATALOG_PROP_WAREHOUSE) .cloned() .unwrap_or_default(); } - self.0.props = props + self.catalog_config().props = props .into_iter() .filter(|(k, _)| { k != HMS_CATALOG_PROP_URI @@ -104,24 +116,25 @@ impl CatalogBuilder for HmsCatalogBuilder { }) .collect(); + let config = self.into_config(); let result = { - if self.0.name.is_none() { + if config.name.is_none() { Err(Error::new( ErrorKind::DataInvalid, "Catalog name is required", )) - } else if self.0.address.is_empty() { + } else if config.address.is_empty() { Err(Error::new( ErrorKind::DataInvalid, "Catalog address is required", )) - } else if self.0.warehouse.is_empty() { + } else if config.warehouse.is_empty() { Err(Error::new( ErrorKind::DataInvalid, "Catalog warehouse is required", )) } else { - HmsCatalog::new(self.0) + HmsCatalog::new(config) } }; diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 39553f7554..ed9c285dad 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -71,6 +71,24 @@ impl Default for RestCatalogBuilder { } } +impl RestCatalogBuilder { + /// Get a mutable reference to the catalog configuration. + pub(crate) fn catalog_config(&mut self) -> &mut RestCatalogConfig { + &mut self.0 + } + + /// Consume the builder and return the catalog configuration. + pub(crate) fn into_config(self) -> RestCatalogConfig { + self.0 + } + + /// Configures the catalog with a custom HTTP client. + pub fn with_client(mut self, client: Client) -> Self { + self.catalog_config().client = Some(client); + self + } +} + impl CatalogBuilder for RestCatalogBuilder { type C = RestCatalog; @@ -79,38 +97,39 @@ impl CatalogBuilder for RestCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.catalog_config().name = Some(name.into()); if props.contains_key(REST_CATALOG_PROP_URI) { - self.0.uri = props + self.catalog_config().uri = props .get(REST_CATALOG_PROP_URI) .cloned() .unwrap_or_default(); } if props.contains_key(REST_CATALOG_PROP_WAREHOUSE) { - self.0.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned() + self.catalog_config().warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned() } // Collect other remaining properties - self.0.props = props + self.catalog_config().props = props .into_iter() .filter(|(k, _)| k != REST_CATALOG_PROP_URI && k != REST_CATALOG_PROP_WAREHOUSE) .collect(); + let config = self.into_config(); let result = { - if self.0.name.is_none() { + if config.name.is_none() { Err(Error::new( ErrorKind::DataInvalid, "Catalog name is required", )) - } else if self.0.uri.is_empty() { + } else if config.uri.is_empty() { Err(Error::new( ErrorKind::DataInvalid, "Catalog uri is required", )) } else { - Ok(RestCatalog::new(self.0)) + Ok(RestCatalog::new(config)) } }; @@ -118,14 +137,6 @@ impl CatalogBuilder for RestCatalogBuilder { } } -impl RestCatalogBuilder { - /// Configures the catalog with a custom HTTP client. - pub fn with_client(mut self, client: Client) -> Self { - self.0.client = Some(client); - self - } -} - /// Rest catalog configuration. #[derive(Clone, Debug, TypedBuilder)] pub(crate) struct RestCatalogConfig { diff --git a/crates/catalog/s3tables/src/catalog.rs b/crates/catalog/s3tables/src/catalog.rs index 3606fac99a..c8c5124290 100644 --- a/crates/catalog/s3tables/src/catalog.rs +++ b/crates/catalog/s3tables/src/catalog.rs @@ -42,7 +42,7 @@ pub const S3TABLES_CATALOG_PROP_ENDPOINT_URL: &str = "endpoint_url"; /// S3Tables catalog configuration. #[derive(Debug)] -struct S3TablesCatalogConfig { +pub(crate) struct S3TablesCatalogConfig { /// Catalog name. name: Option, /// Unlike other buckets, S3Tables bucket is not a physical bucket, but a virtual bucket @@ -82,6 +82,16 @@ impl Default for S3TablesCatalogBuilder { /// Builder methods for [`S3TablesCatalog`]. impl S3TablesCatalogBuilder { + /// Get a mutable reference to the catalog configuration. + pub(crate) fn catalog_config(&mut self) -> &mut S3TablesCatalogConfig { + &mut self.0 + } + + /// Consume the builder and return the catalog configuration. + pub(crate) fn into_config(self) -> S3TablesCatalogConfig { + self.0 + } + /// Configure the catalog with a custom endpoint URL (useful for local testing/mocking). /// /// # Behavior with Properties @@ -91,13 +101,13 @@ impl S3TablesCatalogBuilder { /// This follows the general pattern where properties specified in the `load()` method /// have higher priority than builder method configurations. pub fn with_endpoint_url(mut self, endpoint_url: impl Into) -> Self { - self.0.endpoint_url = Some(endpoint_url.into()); + self.catalog_config().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.catalog_config().client = Some(client); self } @@ -110,7 +120,7 @@ impl S3TablesCatalogBuilder { /// This follows the general pattern where properties specified in the `load()` method /// have higher priority than builder method configurations. pub fn with_table_bucket_arn(mut self, table_bucket_arn: impl Into) -> Self { - self.0.table_bucket_arn = table_bucket_arn.into(); + self.catalog_config().table_bucket_arn = table_bucket_arn.into(); self } } @@ -124,21 +134,21 @@ impl CatalogBuilder for S3TablesCatalogBuilder { props: HashMap, ) -> impl Future> + Send { let catalog_name = name.into(); - self.0.name = Some(catalog_name.clone()); + self.catalog_config().name = Some(catalog_name.clone()); if props.contains_key(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN) { - self.0.table_bucket_arn = props + self.catalog_config().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(); + self.catalog_config().endpoint_url = props.get(S3TABLES_CATALOG_PROP_ENDPOINT_URL).cloned(); } // Collect other remaining properties - self.0.props = props + self.catalog_config().props = props .into_iter() .filter(|(k, _)| { k != S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN @@ -146,19 +156,20 @@ impl CatalogBuilder for S3TablesCatalogBuilder { }) .collect(); + let config = self.into_config(); async move { if catalog_name.trim().is_empty() { Err(Error::new( ErrorKind::DataInvalid, "Catalog name cannot be empty", )) - } else if self.0.table_bucket_arn.is_empty() { + } else if config.table_bucket_arn.is_empty() { Err(Error::new( ErrorKind::DataInvalid, "Table bucket ARN is required", )) } else { - S3TablesCatalog::new(self.0).await + S3TablesCatalog::new(config).await } } } diff --git a/crates/catalog/sql/src/catalog.rs b/crates/catalog/sql/src/catalog.rs index 77b35a228f..1bda1f94cc 100644 --- a/crates/catalog/sql/src/catalog.rs +++ b/crates/catalog/sql/src/catalog.rs @@ -78,12 +78,22 @@ impl Default for SqlCatalogBuilder { } impl SqlCatalogBuilder { + /// Get a mutable reference to the catalog configuration. + pub(crate) fn catalog_config(&mut self) -> &mut SqlCatalogConfig { + &mut self.0 + } + + /// Consume the builder and return the catalog configuration. + pub(crate) fn into_config(self) -> SqlCatalogConfig { + self.0 + } + /// Configure the database URI /// /// If `SQL_CATALOG_PROP_URI` has a value set in `props` during `SqlCatalogBuilder::load`, /// that value takes precedence, and the value specified by this method will not be used. pub fn uri(mut self, uri: impl Into) -> Self { - self.0.uri = uri.into(); + self.catalog_config().uri = uri.into(); self } @@ -92,7 +102,7 @@ impl SqlCatalogBuilder { /// If `SQL_CATALOG_PROP_WAREHOUSE` has a value set in `props` during `SqlCatalogBuilder::load`, /// that value takes precedence, and the value specified by this method will not be used. pub fn warehouse_location(mut self, location: impl Into) -> Self { - self.0.warehouse_location = location.into(); + self.catalog_config().warehouse_location = location.into(); self } @@ -101,7 +111,7 @@ impl SqlCatalogBuilder { /// If `SQL_CATALOG_PROP_BIND_STYLE` has a value set in `props` during `SqlCatalogBuilder::load`, /// that value takes precedence, and the value specified by this method will not be used. pub fn sql_bind_style(mut self, sql_bind_style: SqlBindStyle) -> Self { - self.0.sql_bind_style = sql_bind_style; + self.catalog_config().sql_bind_style = sql_bind_style; self } @@ -111,7 +121,7 @@ impl SqlCatalogBuilder { /// those values will take precedence. pub fn props(mut self, props: HashMap) -> Self { for (k, v) in props { - self.0.props.insert(k, v); + self.catalog_config().props.insert(k, v); } self } @@ -123,7 +133,7 @@ impl SqlCatalogBuilder { /// If the same key has values set in `props` during `SqlCatalogBuilder::load`, /// those values will take precedence. pub fn prop(mut self, key: impl Into, value: impl Into) -> Self { - self.0.props.insert(key.into(), value.into()); + self.catalog_config().props.insert(key.into(), value.into()); self } } @@ -139,25 +149,26 @@ impl CatalogBuilder for SqlCatalogBuilder { let name = name.into(); for (k, v) in props { - self.0.props.insert(k, v); + self.catalog_config().props.insert(k, v); } - if let Some(uri) = self.0.props.remove(SQL_CATALOG_PROP_URI) { - self.0.uri = uri; + if let Some(uri) = self.catalog_config().props.remove(SQL_CATALOG_PROP_URI) { + self.catalog_config().uri = uri; } - if let Some(warehouse_location) = self.0.props.remove(SQL_CATALOG_PROP_WAREHOUSE) { - self.0.warehouse_location = warehouse_location; + if let Some(warehouse_location) = self.catalog_config().props.remove(SQL_CATALOG_PROP_WAREHOUSE) { + self.catalog_config().warehouse_location = warehouse_location; } let mut valid_sql_bind_style = true; - if let Some(sql_bind_style) = self.0.props.remove(SQL_CATALOG_PROP_BIND_STYLE) { + if let Some(sql_bind_style) = self.catalog_config().props.remove(SQL_CATALOG_PROP_BIND_STYLE) { if let Ok(sql_bind_style) = SqlBindStyle::from_str(&sql_bind_style) { - self.0.sql_bind_style = sql_bind_style; + self.catalog_config().sql_bind_style = sql_bind_style; } else { valid_sql_bind_style = false; } } + let config = self.into_config(); async move { if name.trim().is_empty() { Err(Error::new( @@ -175,7 +186,7 @@ impl CatalogBuilder for SqlCatalogBuilder { ), )) } else { - SqlCatalog::new(self.0).await + SqlCatalog::new(config).await } } } @@ -190,7 +201,7 @@ impl CatalogBuilder for SqlCatalogBuilder { /// - `SqlBindStyle::DollarNumeric`: Binds SQL statements using `$1`, `$2`, etc., as placeholders. This is for PostgreSQL databases. /// - `SqlBindStyle::QuestionMark`: Binds SQL statements using `?` as a placeholder. This is for MySQL and SQLite databases. #[derive(Debug)] -struct SqlCatalogConfig { +pub(crate) struct SqlCatalogConfig { uri: String, name: String, warehouse_location: String, diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index cfa3dc6b52..99709f8998 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -52,6 +52,18 @@ impl Default for MemoryCatalogBuilder { } } +impl MemoryCatalogBuilder { + /// Get a mutable reference to the catalog configuration. + pub(crate) fn catalog_config(&mut self) -> &mut MemoryCatalogConfig { + &mut self.0 + } + + /// Consume the builder and return the catalog configuration. + pub(crate) fn into_config(self) -> MemoryCatalogConfig { + self.0 + } +} + impl CatalogBuilder for MemoryCatalogBuilder { type C = MemoryCatalog; @@ -60,34 +72,35 @@ impl CatalogBuilder for MemoryCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.catalog_config().name = Some(name.into()); if props.contains_key(MEMORY_CATALOG_WAREHOUSE) { - self.0.warehouse = props + self.catalog_config().warehouse = props .get(MEMORY_CATALOG_WAREHOUSE) .cloned() .unwrap_or_default() } // Collect other remaining properties - self.0.props = props + self.catalog_config().props = props .into_iter() .filter(|(k, _)| k != MEMORY_CATALOG_WAREHOUSE) .collect(); + let config = self.into_config(); let result = { - if self.0.name.is_none() { + if config.name.is_none() { Err(Error::new( ErrorKind::DataInvalid, "Catalog name is required", )) - } else if self.0.warehouse.is_empty() { + } else if config.warehouse.is_empty() { Err(Error::new( ErrorKind::DataInvalid, "Catalog warehouse is required", )) } else { - MemoryCatalog::new(self.0) + MemoryCatalog::new(config) } }; From 104ae3275650714100cb3f6357cddbad7576e3f1 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Wed, 19 Nov 2025 01:13:22 -0500 Subject: [PATCH 2/6] fmt --- crates/catalog/hms/src/catalog.rs | 3 ++- crates/catalog/s3tables/src/catalog.rs | 3 ++- crates/catalog/sql/src/catalog.rs | 12 ++++++++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/catalog/hms/src/catalog.rs b/crates/catalog/hms/src/catalog.rs index ff57921727..1878a74984 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -89,7 +89,8 @@ impl CatalogBuilder for HmsCatalogBuilder { self.catalog_config().name = Some(name.into()); if props.contains_key(HMS_CATALOG_PROP_URI) { - self.catalog_config().address = props.get(HMS_CATALOG_PROP_URI).cloned().unwrap_or_default(); + self.catalog_config().address = + props.get(HMS_CATALOG_PROP_URI).cloned().unwrap_or_default(); } if let Some(tt) = props.get(HMS_CATALOG_PROP_THRIFT_TRANSPORT) { diff --git a/crates/catalog/s3tables/src/catalog.rs b/crates/catalog/s3tables/src/catalog.rs index c8c5124290..b06cba6aa7 100644 --- a/crates/catalog/s3tables/src/catalog.rs +++ b/crates/catalog/s3tables/src/catalog.rs @@ -144,7 +144,8 @@ impl CatalogBuilder for S3TablesCatalogBuilder { } if props.contains_key(S3TABLES_CATALOG_PROP_ENDPOINT_URL) { - self.catalog_config().endpoint_url = props.get(S3TABLES_CATALOG_PROP_ENDPOINT_URL).cloned(); + self.catalog_config().endpoint_url = + props.get(S3TABLES_CATALOG_PROP_ENDPOINT_URL).cloned(); } // Collect other remaining properties diff --git a/crates/catalog/sql/src/catalog.rs b/crates/catalog/sql/src/catalog.rs index 1bda1f94cc..ee89324332 100644 --- a/crates/catalog/sql/src/catalog.rs +++ b/crates/catalog/sql/src/catalog.rs @@ -155,12 +155,20 @@ impl CatalogBuilder for SqlCatalogBuilder { if let Some(uri) = self.catalog_config().props.remove(SQL_CATALOG_PROP_URI) { self.catalog_config().uri = uri; } - if let Some(warehouse_location) = self.catalog_config().props.remove(SQL_CATALOG_PROP_WAREHOUSE) { + if let Some(warehouse_location) = self + .catalog_config() + .props + .remove(SQL_CATALOG_PROP_WAREHOUSE) + { self.catalog_config().warehouse_location = warehouse_location; } let mut valid_sql_bind_style = true; - if let Some(sql_bind_style) = self.catalog_config().props.remove(SQL_CATALOG_PROP_BIND_STYLE) { + if let Some(sql_bind_style) = self + .catalog_config() + .props + .remove(SQL_CATALOG_PROP_BIND_STYLE) + { if let Ok(sql_bind_style) = SqlBindStyle::from_str(&sql_bind_style) { self.catalog_config().sql_bind_style = sql_bind_style; } else { From 400c6b67e4c2767e95aa37fc6918fb306bde10b6 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Thu, 20 Nov 2025 16:01:28 -0500 Subject: [PATCH 3/6] Change to use Option for Builder --- crates/catalog/glue/src/catalog.rs | 71 ++++++++-------- crates/catalog/hms/src/catalog.rs | 98 +++++++++++----------- crates/catalog/rest/src/catalog.rs | 76 ++++++++--------- crates/catalog/s3tables/src/catalog.rs | 74 +++++++++-------- crates/catalog/sql/src/catalog.rs | 109 +++++++++++++++---------- 5 files changed, 231 insertions(+), 197 deletions(-) diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs index b33bc39a7b..55010559a3 100644 --- a/crates/catalog/glue/src/catalog.rs +++ b/crates/catalog/glue/src/catalog.rs @@ -51,29 +51,23 @@ pub const GLUE_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Builder for [`GlueCatalog`]. #[derive(Debug)] -pub struct GlueCatalogBuilder(GlueCatalogConfig); +pub struct GlueCatalogBuilder { + name: Option, + uri: Option, + catalog_id: Option, + warehouse: Option, + props: HashMap, +} impl Default for GlueCatalogBuilder { fn default() -> Self { - Self(GlueCatalogConfig { + Self { name: None, uri: None, catalog_id: None, - warehouse: "".to_string(), + warehouse: None, props: HashMap::new(), - }) - } -} - -impl GlueCatalogBuilder { - /// Get a mutable reference to the catalog configuration. - pub(crate) fn catalog_config(&mut self) -> &mut GlueCatalogConfig { - &mut self.0 - } - - /// Consume the builder and return the catalog configuration. - pub(crate) fn into_config(self) -> GlueCatalogConfig { - self.0 + } } } @@ -85,25 +79,22 @@ impl CatalogBuilder for GlueCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.catalog_config().name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(GLUE_CATALOG_PROP_URI) { - self.catalog_config().uri = props.get(GLUE_CATALOG_PROP_URI).cloned() + self.uri = props.get(GLUE_CATALOG_PROP_URI).cloned(); } if props.contains_key(GLUE_CATALOG_PROP_CATALOG_ID) { - self.catalog_config().catalog_id = props.get(GLUE_CATALOG_PROP_CATALOG_ID).cloned() + self.catalog_id = props.get(GLUE_CATALOG_PROP_CATALOG_ID).cloned(); } if props.contains_key(GLUE_CATALOG_PROP_WAREHOUSE) { - self.catalog_config().warehouse = props - .get(GLUE_CATALOG_PROP_WAREHOUSE) - .cloned() - .unwrap_or_default(); + self.warehouse = props.get(GLUE_CATALOG_PROP_WAREHOUSE).cloned(); } // Collect other remaining properties - self.catalog_config().props = props + self.props = props .into_iter() .filter(|(k, _)| { k != GLUE_CATALOG_PROP_URI @@ -112,30 +103,40 @@ impl CatalogBuilder for GlueCatalogBuilder { }) .collect(); - let config = self.into_config(); async move { - if config.name.is_none() { - return Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )); - } - if config.warehouse.is_empty() { + // Catalog name and warehouse are required + let name = self.name.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog name is required") + })?; + let warehouse = self.warehouse.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog warehouse is required") + })?; + + if warehouse.is_empty() { return Err(Error::new( ErrorKind::DataInvalid, - "Catalog warehouse is required", + "Catalog warehouse cannot be empty", )); } + let config = GlueCatalogConfig { + name, + uri: self.uri, + catalog_id: self.catalog_id, + warehouse, + props: self.props, + }; + GlueCatalog::new(config).await } } } -#[derive(Debug)] /// Glue Catalog configuration +#[derive(Debug)] pub(crate) struct GlueCatalogConfig { - name: Option, + #[allow(dead_code)] // can be used for debugging + name: String, uri: Option, catalog_id: Option, warehouse: String, diff --git a/crates/catalog/hms/src/catalog.rs b/crates/catalog/hms/src/catalog.rs index 1878a74984..8e49e3ab56 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -50,31 +50,25 @@ pub const THRIFT_TRANSPORT_BUFFERED: &str = "buffered"; /// HMS Catalog warehouse location pub const HMS_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; -/// Builder for [`RestCatalog`]. +/// Builder for [`HmsCatalog`]. #[derive(Debug)] -pub struct HmsCatalogBuilder(HmsCatalogConfig); +pub struct HmsCatalogBuilder { + name: Option, + address: Option, + thrift_transport: HmsThriftTransport, + warehouse: Option, + props: HashMap, +} impl Default for HmsCatalogBuilder { fn default() -> Self { - Self(HmsCatalogConfig { + Self { name: None, - address: "".to_string(), + address: None, thrift_transport: HmsThriftTransport::default(), - warehouse: "".to_string(), + warehouse: None, props: HashMap::new(), - }) - } -} - -impl HmsCatalogBuilder { - /// Get a mutable reference to the catalog configuration. - pub(crate) fn catalog_config(&mut self) -> &mut HmsCatalogConfig { - &mut self.0 - } - - /// Consume the builder and return the catalog configuration. - pub(crate) fn into_config(self) -> HmsCatalogConfig { - self.0 + } } } @@ -86,15 +80,14 @@ impl CatalogBuilder for HmsCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.catalog_config().name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(HMS_CATALOG_PROP_URI) { - self.catalog_config().address = - props.get(HMS_CATALOG_PROP_URI).cloned().unwrap_or_default(); + self.address = props.get(HMS_CATALOG_PROP_URI).cloned(); } if let Some(tt) = props.get(HMS_CATALOG_PROP_THRIFT_TRANSPORT) { - self.catalog_config().thrift_transport = match tt.to_lowercase().as_str() { + self.thrift_transport = match tt.to_lowercase().as_str() { THRIFT_TRANSPORT_FRAMED => HmsThriftTransport::Framed, THRIFT_TRANSPORT_BUFFERED => HmsThriftTransport::Buffered, _ => HmsThriftTransport::default(), @@ -102,13 +95,10 @@ impl CatalogBuilder for HmsCatalogBuilder { } if props.contains_key(HMS_CATALOG_PROP_WAREHOUSE) { - self.catalog_config().warehouse = props - .get(HMS_CATALOG_PROP_WAREHOUSE) - .cloned() - .unwrap_or_default(); + self.warehouse = props.get(HMS_CATALOG_PROP_WAREHOUSE).cloned(); } - self.catalog_config().props = props + self.props = props .into_iter() .filter(|(k, _)| { k != HMS_CATALOG_PROP_URI @@ -117,29 +107,44 @@ impl CatalogBuilder for HmsCatalogBuilder { }) .collect(); - let config = self.into_config(); - let result = { - if config.name.is_none() { - Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )) - } else if config.address.is_empty() { - Err(Error::new( + async move { + let name = self.name.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog name is required") + })?; + + let address = self.address.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog address is required") + })?; + + let warehouse = self.warehouse.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog warehouse is required") + })?; + + + if address.is_empty() { + return Err(Error::new( ErrorKind::DataInvalid, - "Catalog address is required", - )) - } else if config.warehouse.is_empty() { - Err(Error::new( + "Catalog address cannot be empty", + )); + } + + if warehouse.is_empty() { + return Err(Error::new( ErrorKind::DataInvalid, - "Catalog warehouse is required", - )) - } else { - HmsCatalog::new(config) + "Catalog warehouse cannot be empty", + )); } - }; - std::future::ready(result) + let config = HmsCatalogConfig { + name: Some(name), + address, + thrift_transport: self.thrift_transport, + warehouse, + props: self.props, + }; + + HmsCatalog::new(config) + } } } @@ -157,6 +162,7 @@ pub enum HmsThriftTransport { /// Hive metastore Catalog configuration. #[derive(Debug)] pub(crate) struct HmsCatalogConfig { + #[allow(dead_code)] // Stored for debugging and potential future use name: Option, address: String, thrift_transport: HmsThriftTransport, diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index ed9c285dad..196b3d3d1c 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -57,34 +57,30 @@ const PATH_V1: &str = "v1"; /// Builder for [`RestCatalog`]. #[derive(Debug)] -pub struct RestCatalogBuilder(RestCatalogConfig); +pub struct RestCatalogBuilder { + name: Option, + uri: Option, + warehouse: Option, + props: HashMap, + client: Option, +} impl Default for RestCatalogBuilder { fn default() -> Self { - Self(RestCatalogConfig { + Self { name: None, - uri: "".to_string(), + uri: None, warehouse: None, props: HashMap::new(), client: None, - }) + } } } impl RestCatalogBuilder { - /// Get a mutable reference to the catalog configuration. - pub(crate) fn catalog_config(&mut self) -> &mut RestCatalogConfig { - &mut self.0 - } - - /// Consume the builder and return the catalog configuration. - pub(crate) fn into_config(self) -> RestCatalogConfig { - self.0 - } - /// Configures the catalog with a custom HTTP client. pub fn with_client(mut self, client: Client) -> Self { - self.catalog_config().client = Some(client); + self.client = Some(client); self } } @@ -97,49 +93,55 @@ impl CatalogBuilder for RestCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.catalog_config().name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(REST_CATALOG_PROP_URI) { - self.catalog_config().uri = props - .get(REST_CATALOG_PROP_URI) - .cloned() - .unwrap_or_default(); + self.uri = props.get(REST_CATALOG_PROP_URI).cloned(); } if props.contains_key(REST_CATALOG_PROP_WAREHOUSE) { - self.catalog_config().warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned() + self.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned(); } // Collect other remaining properties - self.catalog_config().props = props + self.props = props .into_iter() .filter(|(k, _)| k != REST_CATALOG_PROP_URI && k != REST_CATALOG_PROP_WAREHOUSE) .collect(); - let config = self.into_config(); - let result = { - if config.name.is_none() { - Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )) - } else if config.uri.is_empty() { - Err(Error::new( + async move { + let name = self.name.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog name is required") + })?; + + let uri = self.uri.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog uri is required") + })?; + + if uri.is_empty() { + return Err(Error::new( ErrorKind::DataInvalid, - "Catalog uri is required", - )) - } else { - Ok(RestCatalog::new(config)) + "Catalog uri cannot be empty", + )); } - }; - std::future::ready(result) + let config = RestCatalogConfig { + name: Some(name), + uri, + warehouse: self.warehouse, + props: self.props, + client: self.client, + }; + + Ok(RestCatalog::new(config)) + } } } /// Rest catalog configuration. #[derive(Clone, Debug, TypedBuilder)] pub(crate) struct RestCatalogConfig { + #[allow(dead_code)] // Stored for debugging and potential future use #[builder(default, setter(strip_option))] name: Option, diff --git a/crates/catalog/s3tables/src/catalog.rs b/crates/catalog/s3tables/src/catalog.rs index b06cba6aa7..e8f6c25ad0 100644 --- a/crates/catalog/s3tables/src/catalog.rs +++ b/crates/catalog/s3tables/src/catalog.rs @@ -44,6 +44,7 @@ pub const S3TABLES_CATALOG_PROP_ENDPOINT_URL: &str = "endpoint_url"; #[derive(Debug)] pub(crate) struct S3TablesCatalogConfig { /// Catalog name. + #[allow(dead_code)] // Stored for debugging and potential future use name: Option, /// 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 @@ -65,33 +66,29 @@ pub(crate) struct S3TablesCatalogConfig { /// Builder for [`S3TablesCatalog`]. #[derive(Debug)] -pub struct S3TablesCatalogBuilder(S3TablesCatalogConfig); +pub struct S3TablesCatalogBuilder { + name: Option, + table_bucket_arn: Option, + endpoint_url: Option, + client: Option, + props: HashMap, +} /// Default builder for [`S3TablesCatalog`]. impl Default for S3TablesCatalogBuilder { fn default() -> Self { - Self(S3TablesCatalogConfig { + Self { name: None, - table_bucket_arn: "".to_string(), + table_bucket_arn: None, endpoint_url: None, client: None, props: HashMap::new(), - }) + } } } /// Builder methods for [`S3TablesCatalog`]. impl S3TablesCatalogBuilder { - /// Get a mutable reference to the catalog configuration. - pub(crate) fn catalog_config(&mut self) -> &mut S3TablesCatalogConfig { - &mut self.0 - } - - /// Consume the builder and return the catalog configuration. - pub(crate) fn into_config(self) -> S3TablesCatalogConfig { - self.0 - } - /// Configure the catalog with a custom endpoint URL (useful for local testing/mocking). /// /// # Behavior with Properties @@ -101,13 +98,13 @@ impl S3TablesCatalogBuilder { /// This follows the general pattern where properties specified in the `load()` method /// have higher priority than builder method configurations. pub fn with_endpoint_url(mut self, endpoint_url: impl Into) -> Self { - self.catalog_config().endpoint_url = Some(endpoint_url.into()); + self.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.catalog_config().client = Some(client); + self.client = Some(client); self } @@ -120,7 +117,7 @@ impl S3TablesCatalogBuilder { /// This follows the general pattern where properties specified in the `load()` method /// have higher priority than builder method configurations. pub fn with_table_bucket_arn(mut self, table_bucket_arn: impl Into) -> Self { - self.catalog_config().table_bucket_arn = table_bucket_arn.into(); + self.table_bucket_arn = Some(table_bucket_arn.into()); self } } @@ -134,22 +131,18 @@ impl CatalogBuilder for S3TablesCatalogBuilder { props: HashMap, ) -> impl Future> + Send { let catalog_name = name.into(); - self.catalog_config().name = Some(catalog_name.clone()); + self.name = Some(catalog_name.clone()); if props.contains_key(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN) { - self.catalog_config().table_bucket_arn = props - .get(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN) - .cloned() - .unwrap_or_default(); + self.table_bucket_arn = props.get(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN).cloned(); } if props.contains_key(S3TABLES_CATALOG_PROP_ENDPOINT_URL) { - self.catalog_config().endpoint_url = - props.get(S3TABLES_CATALOG_PROP_ENDPOINT_URL).cloned(); + self.endpoint_url = props.get(S3TABLES_CATALOG_PROP_ENDPOINT_URL).cloned(); } // Collect other remaining properties - self.catalog_config().props = props + self.props = props .into_iter() .filter(|(k, _)| { k != S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN @@ -157,21 +150,34 @@ impl CatalogBuilder for S3TablesCatalogBuilder { }) .collect(); - let config = self.into_config(); async move { if catalog_name.trim().is_empty() { - Err(Error::new( + return Err(Error::new( ErrorKind::DataInvalid, "Catalog name cannot be empty", - )) - } else if config.table_bucket_arn.is_empty() { - Err(Error::new( + )); + } + + let table_bucket_arn = self.table_bucket_arn.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Table bucket ARN is required") + })?; + + if table_bucket_arn.is_empty() { + return Err(Error::new( ErrorKind::DataInvalid, - "Table bucket ARN is required", - )) - } else { - S3TablesCatalog::new(config).await + "Table bucket ARN cannot be empty", + )); } + + let config = S3TablesCatalogConfig { + name: Some(catalog_name), + table_bucket_arn, + endpoint_url: self.endpoint_url, + client: self.client, + props: self.props, + }; + + S3TablesCatalog::new(config).await } } } diff --git a/crates/catalog/sql/src/catalog.rs b/crates/catalog/sql/src/catalog.rs index ee89324332..49211af824 100644 --- a/crates/catalog/sql/src/catalog.rs +++ b/crates/catalog/sql/src/catalog.rs @@ -63,37 +63,31 @@ static TEST_BEFORE_ACQUIRE: bool = true; // Default the health-check of each con /// Builder for [`SqlCatalog`] #[derive(Debug)] -pub struct SqlCatalogBuilder(SqlCatalogConfig); +pub struct SqlCatalogBuilder { + uri: Option, + warehouse_location: Option, + sql_bind_style: SqlBindStyle, + props: HashMap, +} impl Default for SqlCatalogBuilder { fn default() -> Self { - Self(SqlCatalogConfig { - uri: "".to_string(), - name: "".to_string(), - warehouse_location: "".to_string(), + Self { + uri: None, + warehouse_location: None, sql_bind_style: SqlBindStyle::DollarNumeric, props: HashMap::new(), - }) + } } } impl SqlCatalogBuilder { - /// Get a mutable reference to the catalog configuration. - pub(crate) fn catalog_config(&mut self) -> &mut SqlCatalogConfig { - &mut self.0 - } - - /// Consume the builder and return the catalog configuration. - pub(crate) fn into_config(self) -> SqlCatalogConfig { - self.0 - } - /// Configure the database URI /// /// If `SQL_CATALOG_PROP_URI` has a value set in `props` during `SqlCatalogBuilder::load`, /// that value takes precedence, and the value specified by this method will not be used. pub fn uri(mut self, uri: impl Into) -> Self { - self.catalog_config().uri = uri.into(); + self.uri = Some(uri.into()); self } @@ -102,7 +96,7 @@ impl SqlCatalogBuilder { /// If `SQL_CATALOG_PROP_WAREHOUSE` has a value set in `props` during `SqlCatalogBuilder::load`, /// that value takes precedence, and the value specified by this method will not be used. pub fn warehouse_location(mut self, location: impl Into) -> Self { - self.catalog_config().warehouse_location = location.into(); + self.warehouse_location = Some(location.into()); self } @@ -111,7 +105,7 @@ impl SqlCatalogBuilder { /// If `SQL_CATALOG_PROP_BIND_STYLE` has a value set in `props` during `SqlCatalogBuilder::load`, /// that value takes precedence, and the value specified by this method will not be used. pub fn sql_bind_style(mut self, sql_bind_style: SqlBindStyle) -> Self { - self.catalog_config().sql_bind_style = sql_bind_style; + self.sql_bind_style = sql_bind_style; self } @@ -121,7 +115,7 @@ impl SqlCatalogBuilder { /// those values will take precedence. pub fn props(mut self, props: HashMap) -> Self { for (k, v) in props { - self.catalog_config().props.insert(k, v); + self.props.insert(k, v); } self } @@ -133,7 +127,7 @@ impl SqlCatalogBuilder { /// If the same key has values set in `props` during `SqlCatalogBuilder::load`, /// those values will take precedence. pub fn prop(mut self, key: impl Into, value: impl Into) -> Self { - self.catalog_config().props.insert(key.into(), value.into()); + self.props.insert(key.into(), value.into()); self } } @@ -148,43 +142,38 @@ impl CatalogBuilder for SqlCatalogBuilder { ) -> impl Future> + Send { let name = name.into(); + // Merge props from load() into builder props for (k, v) in props { - self.catalog_config().props.insert(k, v); + self.props.insert(k, v); } - if let Some(uri) = self.catalog_config().props.remove(SQL_CATALOG_PROP_URI) { - self.catalog_config().uri = uri; + // Extract special properties from props + if let Some(uri) = self.props.remove(SQL_CATALOG_PROP_URI) { + self.uri = Some(uri); } - if let Some(warehouse_location) = self - .catalog_config() - .props - .remove(SQL_CATALOG_PROP_WAREHOUSE) - { - self.catalog_config().warehouse_location = warehouse_location; + if let Some(warehouse_location) = self.props.remove(SQL_CATALOG_PROP_WAREHOUSE) { + self.warehouse_location = Some(warehouse_location); } let mut valid_sql_bind_style = true; - if let Some(sql_bind_style) = self - .catalog_config() - .props - .remove(SQL_CATALOG_PROP_BIND_STYLE) - { - if let Ok(sql_bind_style) = SqlBindStyle::from_str(&sql_bind_style) { - self.catalog_config().sql_bind_style = sql_bind_style; + if let Some(sql_bind_style_str) = self.props.remove(SQL_CATALOG_PROP_BIND_STYLE) { + if let Ok(sql_bind_style) = SqlBindStyle::from_str(&sql_bind_style_str) { + self.sql_bind_style = sql_bind_style; } else { valid_sql_bind_style = false; } } - let config = self.into_config(); async move { if name.trim().is_empty() { - Err(Error::new( + return Err(Error::new( ErrorKind::DataInvalid, "Catalog name cannot be empty", - )) - } else if !valid_sql_bind_style { - Err(Error::new( + )); + } + + if !valid_sql_bind_style { + return Err(Error::new( ErrorKind::DataInvalid, format!( "`{}` values are valid only if they're `{}` or `{}`", @@ -192,10 +181,40 @@ impl CatalogBuilder for SqlCatalogBuilder { SqlBindStyle::DollarNumeric, SqlBindStyle::QMark ), - )) - } else { - SqlCatalog::new(config).await + )); + } + + let uri = self.uri.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog uri is required") + })?; + + if uri.is_empty() { + return Err(Error::new( + ErrorKind::DataInvalid, + "Catalog uri cannot be empty", + )); } + + let warehouse_location = self.warehouse_location.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog warehouse location is required") + })?; + + if warehouse_location.is_empty() { + return Err(Error::new( + ErrorKind::DataInvalid, + "Catalog warehouse location cannot be empty", + )); + } + + let config = SqlCatalogConfig { + uri, + name, + warehouse_location, + sql_bind_style: self.sql_bind_style, + props: self.props, + }; + + SqlCatalog::new(config).await } } } From dc2dde42634d7c9811b5c7a946fb7aabf3e39dad Mon Sep 17 00:00:00 2001 From: Jonathan Date: Thu, 20 Nov 2025 16:02:55 -0500 Subject: [PATCH 4/6] fmt --- crates/catalog/glue/src/catalog.rs | 10 +++++----- crates/catalog/hms/src/catalog.rs | 15 +++++++-------- crates/catalog/rest/src/catalog.rs | 12 ++++++------ crates/catalog/sql/src/catalog.rs | 11 +++++++---- 4 files changed, 25 insertions(+), 23 deletions(-) diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs index 55010559a3..067ab415f8 100644 --- a/crates/catalog/glue/src/catalog.rs +++ b/crates/catalog/glue/src/catalog.rs @@ -105,13 +105,13 @@ impl CatalogBuilder for GlueCatalogBuilder { async move { // Catalog name and warehouse are required - let name = self.name.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog name is required") - })?; + let name = self + .name + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog name is required"))?; let warehouse = self.warehouse.ok_or_else(|| { Error::new(ErrorKind::DataInvalid, "Catalog warehouse is required") })?; - + if warehouse.is_empty() { return Err(Error::new( ErrorKind::DataInvalid, @@ -135,7 +135,7 @@ impl CatalogBuilder for GlueCatalogBuilder { /// Glue Catalog configuration #[derive(Debug)] pub(crate) struct GlueCatalogConfig { - #[allow(dead_code)] // can be used for debugging + #[allow(dead_code)] // can be used for debugging name: String, uri: Option, catalog_id: Option, diff --git a/crates/catalog/hms/src/catalog.rs b/crates/catalog/hms/src/catalog.rs index 8e49e3ab56..7d90a732fd 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -108,26 +108,25 @@ impl CatalogBuilder for HmsCatalogBuilder { .collect(); async move { - let name = self.name.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog name is required") - })?; + let name = self + .name + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog name is required"))?; - let address = self.address.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog address is required") - })?; + let address = self + .address + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog address is required"))?; let warehouse = self.warehouse.ok_or_else(|| { Error::new(ErrorKind::DataInvalid, "Catalog warehouse is required") })?; - if address.is_empty() { return Err(Error::new( ErrorKind::DataInvalid, "Catalog address cannot be empty", )); } - + if warehouse.is_empty() { return Err(Error::new( ErrorKind::DataInvalid, diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 196b3d3d1c..35b0d06b2d 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -110,13 +110,13 @@ impl CatalogBuilder for RestCatalogBuilder { .collect(); async move { - let name = self.name.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog name is required") - })?; + let name = self + .name + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog name is required"))?; - let uri = self.uri.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog uri is required") - })?; + let uri = self + .uri + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog uri is required"))?; if uri.is_empty() { return Err(Error::new( diff --git a/crates/catalog/sql/src/catalog.rs b/crates/catalog/sql/src/catalog.rs index 49211af824..90aaea25ee 100644 --- a/crates/catalog/sql/src/catalog.rs +++ b/crates/catalog/sql/src/catalog.rs @@ -184,9 +184,9 @@ impl CatalogBuilder for SqlCatalogBuilder { )); } - let uri = self.uri.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog uri is required") - })?; + let uri = self + .uri + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog uri is required"))?; if uri.is_empty() { return Err(Error::new( @@ -196,7 +196,10 @@ impl CatalogBuilder for SqlCatalogBuilder { } let warehouse_location = self.warehouse_location.ok_or_else(|| { - Error::new(ErrorKind::DataInvalid, "Catalog warehouse location is required") + Error::new( + ErrorKind::DataInvalid, + "Catalog warehouse location is required", + ) })?; if warehouse_location.is_empty() { From 15e3d2d05bb02e79dbcd02b002ab8bc35a509959 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Thu, 20 Nov 2025 16:37:05 -0500 Subject: [PATCH 5/6] clippy --- crates/catalog/glue/src/catalog.rs | 14 +------------- crates/catalog/hms/src/catalog.rs | 14 +------------- crates/catalog/rest/src/catalog.rs | 14 +------------- 3 files changed, 3 insertions(+), 39 deletions(-) diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs index 067ab415f8..281809155b 100644 --- a/crates/catalog/glue/src/catalog.rs +++ b/crates/catalog/glue/src/catalog.rs @@ -50,7 +50,7 @@ pub const GLUE_CATALOG_PROP_CATALOG_ID: &str = "catalog_id"; pub const GLUE_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Builder for [`GlueCatalog`]. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct GlueCatalogBuilder { name: Option, uri: Option, @@ -59,18 +59,6 @@ pub struct GlueCatalogBuilder { props: HashMap, } -impl Default for GlueCatalogBuilder { - fn default() -> Self { - Self { - name: None, - uri: None, - catalog_id: None, - warehouse: None, - props: HashMap::new(), - } - } -} - impl CatalogBuilder for GlueCatalogBuilder { type C = GlueCatalog; diff --git a/crates/catalog/hms/src/catalog.rs b/crates/catalog/hms/src/catalog.rs index 7d90a732fd..143246ed1d 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -51,7 +51,7 @@ pub const THRIFT_TRANSPORT_BUFFERED: &str = "buffered"; pub const HMS_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Builder for [`HmsCatalog`]. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct HmsCatalogBuilder { name: Option, address: Option, @@ -60,18 +60,6 @@ pub struct HmsCatalogBuilder { props: HashMap, } -impl Default for HmsCatalogBuilder { - fn default() -> Self { - Self { - name: None, - address: None, - thrift_transport: HmsThriftTransport::default(), - warehouse: None, - props: HashMap::new(), - } - } -} - impl CatalogBuilder for HmsCatalogBuilder { type C = HmsCatalog; diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 35b0d06b2d..63cc30e2b1 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -56,7 +56,7 @@ const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); const PATH_V1: &str = "v1"; /// Builder for [`RestCatalog`]. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct RestCatalogBuilder { name: Option, uri: Option, @@ -65,18 +65,6 @@ pub struct RestCatalogBuilder { client: Option, } -impl Default for RestCatalogBuilder { - fn default() -> Self { - Self { - name: None, - uri: None, - warehouse: None, - props: HashMap::new(), - client: None, - } - } -} - impl RestCatalogBuilder { /// Configures the catalog with a custom HTTP client. pub fn with_client(mut self, client: Client) -> Self { From 46091ff93b2e8ece68d72ad52006e3bd78da7c71 Mon Sep 17 00:00:00 2001 From: Jonathan Date: Mon, 24 Nov 2025 19:20:20 -0500 Subject: [PATCH 6/6] Add `Display` --- crates/catalog/glue/src/catalog.rs | 13 ++- crates/catalog/hms/src/catalog.rs | 5 +- crates/catalog/rest/src/catalog.rs | 12 ++- crates/catalog/s3tables/src/catalog.rs | 78 ++++++++-------- crates/iceberg/src/catalog/memory/catalog.rs | 93 +++++++++++--------- 5 files changed, 115 insertions(+), 86 deletions(-) diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs index 281809155b..f7c77fc9ed 100644 --- a/crates/catalog/glue/src/catalog.rs +++ b/crates/catalog/glue/src/catalog.rs @@ -16,7 +16,7 @@ // under the License. use std::collections::HashMap; -use std::fmt::Debug; +use std::fmt::{self, Debug, Display, Formatter}; use anyhow::anyhow; use async_trait::async_trait; @@ -123,7 +123,6 @@ impl CatalogBuilder for GlueCatalogBuilder { /// Glue Catalog configuration #[derive(Debug)] pub(crate) struct GlueCatalogConfig { - #[allow(dead_code)] // can be used for debugging name: String, uri: Option, catalog_id: Option, @@ -131,6 +130,16 @@ pub(crate) struct GlueCatalogConfig { props: HashMap, } +impl Display for GlueCatalogConfig { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!( + f, + "GlueCatalogConfig(name={}, warehouse={})", + self.name, self.warehouse + ) + } +} + struct GlueClient(aws_sdk_glue::Client); /// Glue Catalog diff --git a/crates/catalog/hms/src/catalog.rs b/crates/catalog/hms/src/catalog.rs index 143246ed1d..08ce2a9c7b 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -123,7 +123,7 @@ impl CatalogBuilder for HmsCatalogBuilder { } let config = HmsCatalogConfig { - name: Some(name), + name, address, thrift_transport: self.thrift_transport, warehouse, @@ -149,8 +149,7 @@ pub enum HmsThriftTransport { /// Hive metastore Catalog configuration. #[derive(Debug)] pub(crate) struct HmsCatalogConfig { - #[allow(dead_code)] // Stored for debugging and potential future use - name: Option, + name: String, address: String, thrift_transport: HmsThriftTransport, warehouse: String, diff --git a/crates/catalog/rest/src/catalog.rs b/crates/catalog/rest/src/catalog.rs index 63cc30e2b1..d44f156437 100644 --- a/crates/catalog/rest/src/catalog.rs +++ b/crates/catalog/rest/src/catalog.rs @@ -19,6 +19,7 @@ use std::any::Any; use std::collections::HashMap; +use std::fmt::{self, Display, Formatter}; use std::future::Future; use std::str::FromStr; @@ -129,7 +130,6 @@ impl CatalogBuilder for RestCatalogBuilder { /// Rest catalog configuration. #[derive(Clone, Debug, TypedBuilder)] pub(crate) struct RestCatalogConfig { - #[allow(dead_code)] // Stored for debugging and potential future use #[builder(default, setter(strip_option))] name: Option, @@ -145,6 +145,16 @@ pub(crate) struct RestCatalogConfig { client: Option, } +impl Display for RestCatalogConfig { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + if let Some(name) = &self.name { + write!(f, "RestCatalogConfig(name={}, uri={})", name, self.uri) + } else { + write!(f, "RestCatalogConfig(name=, uri={})", self.uri) + } + } +} + impl RestCatalogConfig { fn url_prefixed(&self, parts: &[&str]) -> String { [&self.uri, PATH_V1] diff --git a/crates/catalog/s3tables/src/catalog.rs b/crates/catalog/s3tables/src/catalog.rs index e8f6c25ad0..4f48beebd1 100644 --- a/crates/catalog/s3tables/src/catalog.rs +++ b/crates/catalog/s3tables/src/catalog.rs @@ -16,6 +16,7 @@ // under the License. use std::collections::HashMap; +use std::fmt::{self, Display, Formatter}; use std::future::Future; use async_trait::async_trait; @@ -44,17 +45,16 @@ pub const S3TABLES_CATALOG_PROP_ENDPOINT_URL: &str = "endpoint_url"; #[derive(Debug)] pub(crate) struct S3TablesCatalogConfig { /// Catalog name. - #[allow(dead_code)] // Stored for debugging and potential future use - name: Option, + name: 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. - endpoint_url: Option, - /// Optional pre-configured AWS SDK client for S3Tables. - client: Option, + endpoint_url: String, + /// Pre-configured AWS SDK client for S3Tables. + client: 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. @@ -64,8 +64,18 @@ pub(crate) struct S3TablesCatalogConfig { props: HashMap, } +impl Display for S3TablesCatalogConfig { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!( + f, + "S3TablesCatalogConfig(name={}, table_bucket_arn={}, endpoint_url={})", + self.name, self.table_bucket_arn, self.endpoint_url + ) + } +} + /// Builder for [`S3TablesCatalog`]. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct S3TablesCatalogBuilder { name: Option, table_bucket_arn: Option, @@ -74,19 +84,6 @@ pub struct S3TablesCatalogBuilder { props: HashMap, } -/// Default builder for [`S3TablesCatalog`]. -impl Default for S3TablesCatalogBuilder { - fn default() -> Self { - Self { - name: None, - table_bucket_arn: None, - 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). @@ -169,11 +166,23 @@ impl CatalogBuilder for S3TablesCatalogBuilder { )); } + let endpoint_url = self.endpoint_url.unwrap_or_default(); + + let client = if let Some(client) = self.client { + client + } else { + let aws_config = create_sdk_config( + &self.props, + if endpoint_url.is_empty() { None } else { Some(endpoint_url.clone()) }, + ).await; + aws_sdk_s3tables::Client::new(&aws_config) + }; + let config = S3TablesCatalogConfig { - name: Some(catalog_name), + name: catalog_name, table_bucket_arn, - endpoint_url: self.endpoint_url, - client: self.client, + endpoint_url, + client, props: self.props, }; @@ -193,14 +202,8 @@ pub struct S3TablesCatalog { impl S3TablesCatalog { /// Creates a new S3Tables catalog. async fn new(config: S3TablesCatalogConfig) -> Result { - let s3tables_client = if let Some(client) = config.client.clone() { - client - } else { - let aws_config = create_sdk_config(&config.props, config.endpoint_url.clone()).await; - aws_sdk_s3tables::Client::new(&aws_config) - }; - let file_io = FileIOBuilder::new("s3").with_props(&config.props).build()?; + let s3tables_client = config.client.clone(); Ok(Self { config, @@ -684,11 +687,14 @@ mod tests { None => return Ok(None), }; + let aws_config = create_sdk_config(&HashMap::new(), None).await; + let client = aws_sdk_s3tables::Client::new(&aws_config); + let config = S3TablesCatalogConfig { - name: None, + name: "test".to_string(), table_bucket_arn, - endpoint_url: None, - client: None, + endpoint_url: String::new(), + client, props: HashMap::new(), }; @@ -993,11 +999,11 @@ mod tests { // Property value should override builder method value assert_eq!( catalog.config.endpoint_url, - Some(property_endpoint.to_string()) + property_endpoint.to_string() ); assert_ne!( catalog.config.endpoint_url, - Some(builder_endpoint.to_string()) + builder_endpoint.to_string() ); } @@ -1017,7 +1023,7 @@ mod tests { assert_eq!( catalog.config.endpoint_url, - Some(builder_endpoint.to_string()) + builder_endpoint.to_string() ); } @@ -1041,7 +1047,7 @@ mod tests { assert_eq!( catalog.config.endpoint_url, - Some(property_endpoint.to_string()) + property_endpoint.to_string() ); } diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index 99709f8998..241e69702c 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -18,6 +18,7 @@ //! This module contains memory catalog implementation. use std::collections::HashMap; +use std::fmt::{self, Display, Formatter}; use async_trait::async_trait; use futures::lock::{Mutex, MutexGuard}; @@ -39,29 +40,11 @@ pub const MEMORY_CATALOG_WAREHOUSE: &str = "warehouse"; const LOCATION: &str = "location"; /// Builder for [`MemoryCatalog`]. -#[derive(Debug)] -pub struct MemoryCatalogBuilder(MemoryCatalogConfig); - -impl Default for MemoryCatalogBuilder { - fn default() -> Self { - Self(MemoryCatalogConfig { - name: None, - warehouse: "".to_string(), - props: HashMap::new(), - }) - } -} - -impl MemoryCatalogBuilder { - /// Get a mutable reference to the catalog configuration. - pub(crate) fn catalog_config(&mut self) -> &mut MemoryCatalogConfig { - &mut self.0 - } - - /// Consume the builder and return the catalog configuration. - pub(crate) fn into_config(self) -> MemoryCatalogConfig { - self.0 - } +#[derive(Debug, Default)] +pub struct MemoryCatalogBuilder { + name: Option, + warehouse: Option, + props: HashMap, } impl CatalogBuilder for MemoryCatalogBuilder { @@ -72,42 +55,46 @@ impl CatalogBuilder for MemoryCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.catalog_config().name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(MEMORY_CATALOG_WAREHOUSE) { - self.catalog_config().warehouse = props - .get(MEMORY_CATALOG_WAREHOUSE) - .cloned() - .unwrap_or_default() + self.warehouse = props.get(MEMORY_CATALOG_WAREHOUSE).cloned(); } // Collect other remaining properties - self.catalog_config().props = props + self.props = props .into_iter() .filter(|(k, _)| k != MEMORY_CATALOG_WAREHOUSE) .collect(); - let config = self.into_config(); - let result = { - if config.name.is_none() { - Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )) - } else if config.warehouse.is_empty() { - Err(Error::new( + async move { + let name = self + .name + .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog name is required"))?; + + let warehouse = self.warehouse.ok_or_else(|| { + Error::new(ErrorKind::DataInvalid, "Catalog warehouse is required") + })?; + + if warehouse.is_empty() { + return Err(Error::new( ErrorKind::DataInvalid, - "Catalog warehouse is required", - )) - } else { - MemoryCatalog::new(config) + "Catalog warehouse cannot be empty", + )); } - }; - std::future::ready(result) + let config = MemoryCatalogConfig { + name: Some(name), + warehouse, + props: self.props, + }; + + MemoryCatalog::new(config) + } } } +/// Memory catalog configuration. #[derive(Clone, Debug)] pub(crate) struct MemoryCatalogConfig { name: Option, @@ -115,6 +102,24 @@ pub(crate) struct MemoryCatalogConfig { props: HashMap, } +impl Display for MemoryCatalogConfig { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + if let Some(name) = &self.name { + write!( + f, + "MemoryCatalogConfig(name={}, warehouse={})", + name, self.warehouse + ) + } else { + write!( + f, + "MemoryCatalogConfig(name=, warehouse={})", + self.warehouse + ) + } + } +} + /// Memory catalog implementation. #[derive(Debug)] pub struct MemoryCatalog {