Add Display

Author: jonathanc-n

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,14 +123,23 @@ 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<String>,
     catalog_id: Option<String>,
     warehouse: String,
     props: HashMap<String, String>,
 }
 
+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

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<String>,
+    name: String,
     address: String,
     thrift_transport: HmsThriftTransport,
     warehouse: String,

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<String>,
 
@@ -145,6 +145,16 @@ pub(crate) struct RestCatalogConfig {
     client: Option<Client>,
 }
 
+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=<none>, uri={})", self.uri)
+        }
+    }
+}
+
 impl RestCatalogConfig {
     fn url_prefixed(&self, parts: &[&str]) -> String {
         [&self.uri, PATH_V1]

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<String>,
+    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<String>,
-    /// Optional pre-configured AWS SDK client for S3Tables.
-    client: Option<aws_sdk_s3tables::Client>,
+    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<String, String>,
 }
 
+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<String>,
     table_bucket_arn: Option<String>,
@@ -74,19 +84,6 @@ pub struct S3TablesCatalogBuilder {
     props: HashMap<String, String>,
 }
 
-/// 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<Self> {
-        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()
         );
     }
 

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<String>,
+    warehouse: Option<String>,
+    props: HashMap<String, String>,
 }
 
 impl CatalogBuilder for MemoryCatalogBuilder {
@@ -72,49 +55,71 @@ impl CatalogBuilder for MemoryCatalogBuilder {
         name: impl Into<String>,
         props: HashMap<String, String>,
     ) -> impl Future<Output = Result<Self::C>> + 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<String>,
     warehouse: String,
     props: HashMap<String, String>,
 }
 
+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=<none>, warehouse={})",
+                self.warehouse
+            )
+        }
+    }
+}
+
 /// Memory catalog implementation.
 #[derive(Debug)]
 pub struct MemoryCatalog {