address comments

Author: sundy-li

crates/catalog/glue/src/catalog.rs

@@ -384,10 +384,12 @@ impl Catalog for GlueCatalog {
             Some(location) => location.clone(),
             None => {
                 let ns = self.get_namespace(namespace).await?;
-                get_default_table_location(&ns, &db_name, &table_name, &self.config.warehouse)
+                let location =
+                    get_default_table_location(&ns, &db_name, &table_name, &self.config.warehouse);
+                creation.location = Some(location.clone());
+                location
             }
         };
-        creation.location = Some(location.clone());
         let metadata = TableMetadataBuilder::from_table_creation(creation)?
             .build()?
             .metadata;

crates/catalog/glue/tests/glue_catalog_test.rs

@@ -105,7 +105,7 @@ async fn set_test_namespace(catalog: &GlueCatalog, namespace: &NamespaceIdent) -
     Ok(())
 }
 
-fn set_table_creation(name: impl ToString) -> Result<TableCreation> {
+fn set_table_creation(location: Option<String>, name: impl ToString) -> Result<TableCreation> {
     let schema = Schema::builder()
         .with_schema_id(0)
         .with_fields(vec![
@@ -114,19 +114,19 @@ fn set_table_creation(name: impl ToString) -> Result<TableCreation> {
         ])
         .build()?;
 
-    let creation = TableCreation::builder()
+    let builder = TableCreation::builder()
         .name(name.to_string())
         .properties(HashMap::new())
-        .schema(schema)
-        .build();
+        .location_opt(location)
+        .schema(schema);
 
-    Ok(creation)
+    Ok(builder.build())
 }
 
 #[tokio::test]
 async fn test_rename_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_rename_table".into()));
 
     catalog
@@ -153,7 +153,7 @@ async fn test_rename_table() -> Result<()> {
 #[tokio::test]
 async fn test_table_exists() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_table_exists".into()));
 
     catalog
@@ -177,7 +177,7 @@ async fn test_table_exists() -> Result<()> {
 #[tokio::test]
 async fn test_drop_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_drop_table".into()));
 
     catalog
@@ -198,7 +198,7 @@ async fn test_drop_table() -> Result<()> {
 #[tokio::test]
 async fn test_load_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_load_table".into()));
 
     catalog
@@ -226,10 +226,8 @@ async fn test_create_table() -> Result<()> {
     let catalog = get_catalog().await;
     let namespace = NamespaceIdent::new("test_create_table".to_string());
     set_test_namespace(&catalog, &namespace).await?;
-    let mut creation = set_table_creation("my_table")?;
     // inject custom location, ignore the namespace prefix
-    creation.location = Some("s3a://warehouse/hive".to_string());
-
+    let creation = set_table_creation(Some("s3a://warehouse/hive".into()), "my_table")?;
     let result = catalog.create_table(&namespace, creation).await?;
 
     assert_eq!(result.identifier().name(), "my_table");

crates/catalog/hms/src/catalog.rs

@@ -342,10 +342,11 @@ impl Catalog for HmsCatalog {
             Some(location) => location.clone(),
             None => {
                 let ns = self.get_namespace(namespace).await?;
-                get_default_table_location(&ns, &table_name, &self.config.warehouse)
+                let location = get_default_table_location(&ns, &table_name, &self.config.warehouse);
+                creation.location = Some(location.clone());
+                location
             }
         };
-        creation.location = Some(location.clone());
         let metadata = TableMetadataBuilder::from_table_creation(creation)?
             .build()?
             .metadata;

crates/catalog/hms/tests/hms_catalog_test.rs

@@ -101,7 +101,7 @@ async fn set_test_namespace(catalog: &HmsCatalog, namespace: &NamespaceIdent) ->
     Ok(())
 }
 
-fn set_table_creation(name: impl ToString) -> Result<TableCreation> {
+fn set_table_creation(location: Option<String>, name: impl ToString) -> Result<TableCreation> {
     let schema = Schema::builder()
         .with_schema_id(0)
         .with_fields(vec![
@@ -110,19 +110,19 @@ fn set_table_creation(name: impl ToString) -> Result<TableCreation> {
         ])
         .build()?;
 
-    let creation = TableCreation::builder()
+    let builder = TableCreation::builder()
         .name(name.to_string())
         .properties(HashMap::new())
-        .schema(schema)
-        .build();
+        .location_opt(location)
+        .schema(schema);
 
-    Ok(creation)
+    Ok(builder.build())
 }
 
 #[tokio::test]
 async fn test_rename_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation: TableCreation = set_table_creation("my_table")?;
+    let creation: TableCreation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_rename_table".into()));
     set_test_namespace(&catalog, namespace.name()).await?;
 
@@ -142,7 +142,7 @@ async fn test_rename_table() -> Result<()> {
 #[tokio::test]
 async fn test_table_exists() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_table_exists".into()));
     set_test_namespace(&catalog, namespace.name()).await?;
 
@@ -158,7 +158,7 @@ async fn test_table_exists() -> Result<()> {
 #[tokio::test]
 async fn test_drop_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_drop_table".into()));
     set_test_namespace(&catalog, namespace.name()).await?;
 
@@ -176,7 +176,7 @@ async fn test_drop_table() -> Result<()> {
 #[tokio::test]
 async fn test_load_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_load_table".into()));
     set_test_namespace(&catalog, namespace.name()).await?;
 
@@ -199,9 +199,8 @@ async fn test_load_table() -> Result<()> {
 #[tokio::test]
 async fn test_create_table() -> Result<()> {
     let catalog = get_catalog().await;
-    let mut creation = set_table_creation("my_table")?;
     // inject custom location, ignore the namespace prefix
-    creation.location = Some("s3a://warehouse/hive".to_string());
+    let creation = set_table_creation(Some("s3a://warehouse/hive".into()), "my_table")?;
     let namespace = Namespace::new(NamespaceIdent::new("test_create_table".into()));
     set_test_namespace(&catalog, namespace.name()).await?;
 
@@ -230,7 +229,7 @@ async fn test_list_tables() -> Result<()> {
 
     assert_eq!(result, vec![]);
 
-    let creation = set_table_creation("my_table")?;
+    let creation = set_table_creation(None, "my_table")?;
     catalog.create_table(ns.name(), creation).await?;
     let result = catalog.list_tables(ns.name()).await?;