diff --git a/crates/catalog/glue/src/catalog.rs b/crates/catalog/glue/src/catalog.rs index 4514f2d7ab..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; @@ -50,19 +50,13 @@ pub const GLUE_CATALOG_PROP_CATALOG_ID: &str = "catalog_id"; pub const GLUE_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; /// Builder for [`GlueCatalog`]. -#[derive(Debug)] -pub struct GlueCatalogBuilder(GlueCatalogConfig); - -impl Default for GlueCatalogBuilder { - fn default() -> Self { - Self(GlueCatalogConfig { - name: None, - uri: None, - catalog_id: None, - warehouse: "".to_string(), - props: HashMap::new(), - }) - } +#[derive(Debug, Default)] +pub struct GlueCatalogBuilder { + name: Option, + uri: Option, + catalog_id: Option, + warehouse: Option, + props: HashMap, } impl CatalogBuilder for GlueCatalogBuilder { @@ -73,25 +67,22 @@ impl CatalogBuilder for GlueCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(GLUE_CATALOG_PROP_URI) { - self.0.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.0.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.0.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.0.props = props + self.props = props .into_iter() .filter(|(k, _)| { k != GLUE_CATALOG_PROP_URI @@ -101,34 +92,54 @@ impl CatalogBuilder for GlueCatalogBuilder { .collect(); async move { - if self.0.name.is_none() { + // 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 name is required", - )); - } - if self.0.warehouse.is_empty() { - return Err(Error::new( - ErrorKind::DataInvalid, - "Catalog warehouse is required", + "Catalog warehouse cannot be empty", )); } - GlueCatalog::new(self.0).await + 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, + name: String, uri: Option, catalog_id: Option, warehouse: String, 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 b7d192210b..08ce2a9c7b 100644 --- a/crates/catalog/hms/src/catalog.rs +++ b/crates/catalog/hms/src/catalog.rs @@ -50,20 +50,14 @@ pub const THRIFT_TRANSPORT_BUFFERED: &str = "buffered"; /// HMS Catalog warehouse location pub const HMS_CATALOG_PROP_WAREHOUSE: &str = "warehouse"; -/// Builder for [`RestCatalog`]. -#[derive(Debug)] -pub struct HmsCatalogBuilder(HmsCatalogConfig); - -impl Default for HmsCatalogBuilder { - fn default() -> Self { - Self(HmsCatalogConfig { - name: None, - address: "".to_string(), - thrift_transport: HmsThriftTransport::default(), - warehouse: "".to_string(), - props: HashMap::new(), - }) - } +/// Builder for [`HmsCatalog`]. +#[derive(Debug, Default)] +pub struct HmsCatalogBuilder { + name: Option, + address: Option, + thrift_transport: HmsThriftTransport, + warehouse: Option, + props: HashMap, } impl CatalogBuilder for HmsCatalogBuilder { @@ -74,14 +68,14 @@ impl CatalogBuilder for HmsCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.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.address = props.get(HMS_CATALOG_PROP_URI).cloned(); } if let Some(tt) = props.get(HMS_CATALOG_PROP_THRIFT_TRANSPORT) { - self.0.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(), @@ -89,13 +83,10 @@ impl CatalogBuilder for HmsCatalogBuilder { } if props.contains_key(HMS_CATALOG_PROP_WAREHOUSE) { - self.0.warehouse = props - .get(HMS_CATALOG_PROP_WAREHOUSE) - .cloned() - .unwrap_or_default(); + self.warehouse = props.get(HMS_CATALOG_PROP_WAREHOUSE).cloned(); } - self.0.props = props + self.props = props .into_iter() .filter(|(k, _)| { k != HMS_CATALOG_PROP_URI @@ -104,28 +95,43 @@ impl CatalogBuilder for HmsCatalogBuilder { }) .collect(); - let result = { - if self.0.name.is_none() { - Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )) - } else if self.0.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 self.0.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(self.0) + "Catalog warehouse cannot be empty", + )); } - }; - std::future::ready(result) + let config = HmsCatalogConfig { + name, + address, + thrift_transport: self.thrift_transport, + warehouse, + props: self.props, + }; + + HmsCatalog::new(config) + } } } @@ -143,7 +149,7 @@ pub enum HmsThriftTransport { /// Hive metastore Catalog configuration. #[derive(Debug)] pub(crate) struct HmsCatalogConfig { - 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 39553f7554..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; @@ -56,18 +57,20 @@ const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); const PATH_V1: &str = "v1"; /// Builder for [`RestCatalog`]. -#[derive(Debug)] -pub struct RestCatalogBuilder(RestCatalogConfig); - -impl Default for RestCatalogBuilder { - fn default() -> Self { - Self(RestCatalogConfig { - name: None, - uri: "".to_string(), - warehouse: None, - props: HashMap::new(), - client: None, - }) +#[derive(Debug, Default)] +pub struct RestCatalogBuilder { + name: Option, + uri: Option, + warehouse: Option, + props: HashMap, + client: Option, +} + +impl RestCatalogBuilder { + /// Configures the catalog with a custom HTTP client. + pub fn with_client(mut self, client: Client) -> Self { + self.client = Some(client); + self } } @@ -79,50 +82,48 @@ impl CatalogBuilder for RestCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(REST_CATALOG_PROP_URI) { - self.0.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.0.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned() + self.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned(); } // Collect other remaining properties - self.0.props = props + self.props = props .into_iter() .filter(|(k, _)| k != REST_CATALOG_PROP_URI && k != REST_CATALOG_PROP_WAREHOUSE) .collect(); - let result = { - if self.0.name.is_none() { - Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )) - } else if self.0.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(self.0)) + "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, + }; -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 + Ok(RestCatalog::new(config)) + } } } @@ -144,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 3606fac99a..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; @@ -42,18 +43,18 @@ 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, + 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. @@ -63,23 +64,26 @@ struct S3TablesCatalogConfig { props: HashMap, } -/// 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(), - }) +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, Default)] +pub struct S3TablesCatalogBuilder { + name: Option, + table_bucket_arn: Option, + endpoint_url: Option, + client: Option, + props: HashMap, +} + /// Builder methods for [`S3TablesCatalog`]. impl S3TablesCatalogBuilder { /// Configure the catalog with a custom endpoint URL (useful for local testing/mocking). @@ -91,13 +95,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.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.client = Some(client); self } @@ -110,7 +114,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.table_bucket_arn = Some(table_bucket_arn.into()); self } } @@ -124,21 +128,18 @@ impl CatalogBuilder for S3TablesCatalogBuilder { props: HashMap, ) -> impl Future> + Send { let catalog_name = name.into(); - self.0.name = Some(catalog_name.clone()); + self.name = Some(catalog_name.clone()); 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(); + self.table_bucket_arn = props.get(S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN).cloned(); } if props.contains_key(S3TABLES_CATALOG_PROP_ENDPOINT_URL) { - self.0.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.0.props = props + self.props = props .into_iter() .filter(|(k, _)| { k != S3TABLES_CATALOG_PROP_TABLE_BUCKET_ARN @@ -148,18 +149,44 @@ impl CatalogBuilder for S3TablesCatalogBuilder { async move { if catalog_name.trim().is_empty() { - Err(Error::new( + return Err(Error::new( ErrorKind::DataInvalid, "Catalog name cannot be empty", - )) - } else if self.0.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(self.0).await + "Table bucket ARN cannot be empty", + )); } + + 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: catalog_name, + table_bucket_arn, + endpoint_url, + client, + props: self.props, + }; + + S3TablesCatalog::new(config).await } } } @@ -175,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, @@ -666,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(), }; @@ -975,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() ); } @@ -999,7 +1023,7 @@ mod tests { assert_eq!( catalog.config.endpoint_url, - Some(builder_endpoint.to_string()) + builder_endpoint.to_string() ); } @@ -1023,7 +1047,7 @@ mod tests { assert_eq!( catalog.config.endpoint_url, - Some(property_endpoint.to_string()) + property_endpoint.to_string() ); } diff --git a/crates/catalog/sql/src/catalog.rs b/crates/catalog/sql/src/catalog.rs index 77b35a228f..90aaea25ee 100644 --- a/crates/catalog/sql/src/catalog.rs +++ b/crates/catalog/sql/src/catalog.rs @@ -63,17 +63,21 @@ 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(), - }) + } } } @@ -83,7 +87,7 @@ impl SqlCatalogBuilder { /// 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.uri = Some(uri.into()); self } @@ -92,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.0.warehouse_location = location.into(); + self.warehouse_location = Some(location.into()); self } @@ -101,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.0.sql_bind_style = sql_bind_style; + self.sql_bind_style = sql_bind_style; self } @@ -111,7 +115,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.props.insert(k, v); } self } @@ -123,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.0.props.insert(key.into(), value.into()); + self.props.insert(key.into(), value.into()); self } } @@ -138,21 +142,23 @@ impl CatalogBuilder for SqlCatalogBuilder { ) -> impl Future> + Send { let name = name.into(); + // Merge props from load() into builder props for (k, v) in props { - self.0.props.insert(k, v); + self.props.insert(k, v); } - if let Some(uri) = self.0.props.remove(SQL_CATALOG_PROP_URI) { - self.0.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.0.props.remove(SQL_CATALOG_PROP_WAREHOUSE) { - self.0.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.0.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; + 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; } @@ -160,12 +166,14 @@ impl CatalogBuilder for SqlCatalogBuilder { 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 `{}`", @@ -173,10 +181,43 @@ impl CatalogBuilder for SqlCatalogBuilder { SqlBindStyle::DollarNumeric, SqlBindStyle::QMark ), - )) - } else { - SqlCatalog::new(self.0).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 } } } @@ -190,7 +231,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..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,17 +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(), - }) - } +#[derive(Debug, Default)] +pub struct MemoryCatalogBuilder { + name: Option, + warehouse: Option, + props: HashMap, } impl CatalogBuilder for MemoryCatalogBuilder { @@ -60,41 +55,46 @@ impl CatalogBuilder for MemoryCatalogBuilder { name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.0.name = Some(name.into()); + self.name = Some(name.into()); if props.contains_key(MEMORY_CATALOG_WAREHOUSE) { - self.0.warehouse = props - .get(MEMORY_CATALOG_WAREHOUSE) - .cloned() - .unwrap_or_default() + self.warehouse = props.get(MEMORY_CATALOG_WAREHOUSE).cloned(); } // Collect other remaining properties - self.0.props = props + self.props = props .into_iter() .filter(|(k, _)| k != MEMORY_CATALOG_WAREHOUSE) .collect(); - let result = { - if self.0.name.is_none() { - Err(Error::new( - ErrorKind::DataInvalid, - "Catalog name is required", - )) - } else if self.0.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(self.0) + "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, @@ -102,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 {