chore: Improve clarity when using CatalogConfigs in CatalogBuilder

crates/catalog/glue/src/catalog.rs

@@ -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<String>,
+    uri: Option<String>,
+    catalog_id: Option<String>,
+    warehouse: Option<String>,
+    props: HashMap<String, String>,
 }
 
 impl CatalogBuilder for GlueCatalogBuilder {
@@ -73,25 +67,22 @@ impl CatalogBuilder for GlueCatalogBuilder {
         name: impl Into<String>,
         props: HashMap<String, String>,
     ) -> impl Future<Output = Result<Self::C>> + 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,28 +92,39 @@ impl CatalogBuilder for GlueCatalogBuilder {
             .collect();
 
         async move {
-            if self.0.name.is_none() {
-                return Err(Error::new(
-                    ErrorKind::DataInvalid,
-                    "Catalog name is required",
-                ));
-            }
-            if self.0.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",
                 ));
             }
 
-            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<String>,
+    #[allow(dead_code)] // can be used for debugging
+    name: String,
     uri: Option<String>,
     catalog_id: Option<String>,
     warehouse: String,

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<String>,
+    address: Option<String>,
+    thrift_transport: HmsThriftTransport,
+    warehouse: Option<String>,
+    props: HashMap<String, String>,
 }
 
 impl CatalogBuilder for HmsCatalogBuilder {
@@ -74,28 +68,25 @@ impl CatalogBuilder for HmsCatalogBuilder {
         name: impl Into<String>,
         props: HashMap<String, String>,
     ) -> impl Future<Output = Result<Self::C>> + 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(),
             };
         }
 
         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: Some(name),
+                address,
+                thrift_transport: self.thrift_transport,
+                warehouse,
+                props: self.props,
+            };
+
+            HmsCatalog::new(config)
+        }
     }
 }
 
@@ -143,6 +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<String>,
     address: String,
     thrift_transport: HmsThriftTransport,

crates/catalog/rest/src/catalog.rs

@@ -56,18 +56,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<String>,
+    uri: Option<String>,
+    warehouse: Option<String>,
+    props: HashMap<String, String>,
+    client: Option<Client>,
+}
+
+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,56 +81,55 @@ impl CatalogBuilder for RestCatalogBuilder {
         name: impl Into<String>,
         props: HashMap<String, String>,
     ) -> impl Future<Output = Result<Self::C>> + 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))
+        }
     }
 }
 
 /// 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<String>,