Refactor: use metadatalocaiton:new_with_table_location in catalog create_table (#1558)

Author: yingjianwu98

Cargo.lock

@@ -38,7 +38,6 @@ serde_json = { workspace = true }
 tokio = { workspace = true }
 tracing = { workspace = true }
 typed-builder = { workspace = true }
-uuid = { workspace = true }
 
 [dev-dependencies]
 ctor = { workspace = true }

crates/catalog/glue/Cargo.toml

@@ -26,15 +26,15 @@ use iceberg::io::{
 use iceberg::spec::{TableMetadata, TableMetadataBuilder};
 use iceberg::table::Table;
 use iceberg::{
-    Catalog, Error, ErrorKind, Namespace, NamespaceIdent, Result, TableCommit, TableCreation,
-    TableIdent,
+    Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent, Result, TableCommit,
+    TableCreation, TableIdent,
 };
 use typed_builder::TypedBuilder;
 
 use crate::error::{from_aws_build_error, from_aws_sdk_error};
 use crate::utils::{
-    convert_to_database, convert_to_glue_table, convert_to_namespace, create_metadata_location,
-    create_sdk_config, get_default_table_location, get_metadata_location, validate_namespace,
+    convert_to_database, convert_to_glue_table, convert_to_namespace, create_sdk_config,
+    get_default_table_location, get_metadata_location, validate_namespace,
 };
 use crate::{
     AWS_ACCESS_KEY_ID, AWS_REGION_NAME, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN, with_catalog_id,
@@ -393,7 +393,8 @@ impl Catalog for GlueCatalog {
         let metadata = TableMetadataBuilder::from_table_creation(creation)?
             .build()?
             .metadata;
-        let metadata_location = create_metadata_location(&location, 0)?;
+        let metadata_location =
+            MetadataLocation::new_with_table_location(location.clone()).to_string();
 
         metadata.write_to(&self.file_io, &metadata_location).await?;
 

crates/catalog/glue/src/catalog.rs

@@ -22,7 +22,6 @@ use aws_sdk_glue::config::Credentials;
 use aws_sdk_glue::types::{Database, DatabaseInput, StorageDescriptor, TableInput};
 use iceberg::spec::TableMetadata;
 use iceberg::{Error, ErrorKind, Namespace, NamespaceIdent, Result};
-use uuid::Uuid;
 
 use crate::error::from_aws_build_error;
 use crate::schema::GlueSchemaBuilder;
@@ -228,30 +227,6 @@ pub(crate) fn get_default_table_location(
     }
 }
 
-/// Create metadata location from `location` and `version`
-pub(crate) fn create_metadata_location(location: impl AsRef<str>, version: i32) -> Result<String> {
-    if version < 0 {
-        return Err(Error::new(
-            ErrorKind::DataInvalid,
-            format!(
-                "Table metadata version: '{}' must be a non-negative integer",
-                version
-            ),
-        ));
-    };
-
-    let version = format!("{:0>5}", version);
-    let id = Uuid::new_v4();
-    let metadata_location = format!(
-        "{}/metadata/{}-{}.metadata.json",
-        location.as_ref(),
-        version,
-        id
-    );
-
-    Ok(metadata_location)
-}
-
 /// Get metadata location from `GlueTable` parameters
 pub(crate) fn get_metadata_location(
     parameters: &Option<HashMap<String, String>>,
@@ -288,7 +263,7 @@ mod tests {
     use aws_sdk_glue::config::ProvideCredentials;
     use aws_sdk_glue::types::Column;
     use iceberg::spec::{NestedField, PrimitiveType, Schema, TableMetadataBuilder, Type};
-    use iceberg::{Namespace, Result, TableCreation};
+    use iceberg::{MetadataLocation, Namespace, Result, TableCreation};
 
     use super::*;
     use crate::schema::{ICEBERG_FIELD_CURRENT, ICEBERG_FIELD_ID, ICEBERG_FIELD_OPTIONAL};
@@ -332,7 +307,7 @@ mod tests {
     fn test_convert_to_glue_table() -> Result<()> {
         let table_name = "my_table".to_string();
         let location = "s3a://warehouse/hive".to_string();
-        let metadata_location = create_metadata_location(location.clone(), 0)?;
+        let metadata_location = MetadataLocation::new_with_table_location(location).to_string();
         let properties = HashMap::new();
         let schema = Schema::builder()
             .with_schema_id(1)
@@ -372,22 +347,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn test_create_metadata_location() -> Result<()> {
-        let location = "my_base_location";
-        let valid_version = 0;
-        let invalid_version = -1;
-
-        let valid_result = create_metadata_location(location, valid_version)?;
-        let invalid_result = create_metadata_location(location, invalid_version);
-
-        assert!(valid_result.starts_with("my_base_location/metadata/00000-"));
-        assert!(valid_result.ends_with(".metadata.json"));
-        assert!(invalid_result.is_err());
-
-        Ok(())
-    }
-
     #[test]
     fn test_get_default_table_location() -> Result<()> {
         let properties = HashMap::from([(LOCATION.to_string(), "db_location".to_string())]);

crates/catalog/glue/src/utils.rs

@@ -39,7 +39,6 @@ serde_json = { workspace = true }
 tokio = { workspace = true }
 tracing = { workspace = true }
 typed-builder = { workspace = true }
-uuid = { workspace = true }
 volo-thrift = { workspace = true }
 
 # Transitive dependencies below

crates/catalog/hms/Cargo.toml

@@ -29,8 +29,8 @@ use iceberg::io::FileIO;
 use iceberg::spec::{TableMetadata, TableMetadataBuilder};
 use iceberg::table::Table;
 use iceberg::{
-    Catalog, Error, ErrorKind, Namespace, NamespaceIdent, Result, TableCommit, TableCreation,
-    TableIdent,
+    Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent, Result, TableCommit,
+    TableCreation, TableIdent,
 };
 use typed_builder::TypedBuilder;
 use volo_thrift::MaybeException;
@@ -351,7 +351,8 @@ impl Catalog for HmsCatalog {
             .build()?
             .metadata;
 
-        let metadata_location = create_metadata_location(&location, 0)?;
+        let metadata_location =
+            MetadataLocation::new_with_table_location(location.clone()).to_string();
 
         metadata.write_to(&self.file_io, &metadata_location).await?;
 

crates/catalog/hms/src/catalog.rs

@@ -22,7 +22,6 @@ use hive_metastore::{Database, PrincipalType, SerDeInfo, StorageDescriptor};
 use iceberg::spec::Schema;
 use iceberg::{Error, ErrorKind, Namespace, NamespaceIdent, Result};
 use pilota::{AHashMap, FastStr};
-use uuid::Uuid;
 
 use crate::schema::HiveSchemaBuilder;
 
@@ -249,30 +248,6 @@ pub(crate) fn get_default_table_location(
     format!("{}/{}", location, table_name.as_ref())
 }
 
-/// Create metadata location from `location` and `version`
-pub(crate) fn create_metadata_location(location: impl AsRef<str>, version: i32) -> Result<String> {
-    if version < 0 {
-        return Err(Error::new(
-            ErrorKind::DataInvalid,
-            format!(
-                "Table metadata version: '{}' must be a non-negative integer",
-                version
-            ),
-        ));
-    };
-
-    let version = format!("{:0>5}", version);
-    let id = Uuid::new_v4();
-    let metadata_location = format!(
-        "{}/metadata/{}-{}.metadata.json",
-        location.as_ref(),
-        version,
-        id
-    );
-
-    Ok(metadata_location)
-}
-
 /// Get metadata location from `HiveTable` parameters
 pub(crate) fn get_metadata_location(
     parameters: &Option<AHashMap<FastStr, FastStr>>,
@@ -339,7 +314,7 @@ fn get_current_time() -> Result<i32> {
 #[cfg(test)]
 mod tests {
     use iceberg::spec::{NestedField, PrimitiveType, Type};
-    use iceberg::{Namespace, NamespaceIdent};
+    use iceberg::{MetadataLocation, Namespace, NamespaceIdent};
 
     use super::*;
 
@@ -370,7 +345,8 @@ mod tests {
         let db_name = "my_db".to_string();
         let table_name = "my_table".to_string();
         let location = "s3a://warehouse/hms".to_string();
-        let metadata_location = create_metadata_location(location.clone(), 0)?;
+        let metadata_location =
+            MetadataLocation::new_with_table_location(location.clone()).to_string();
         let properties = HashMap::new();
         let schema = Schema::builder()
             .with_schema_id(1)
@@ -414,22 +390,6 @@ mod tests {
         Ok(())
     }
 
-    #[test]
-    fn test_create_metadata_location() -> Result<()> {
-        let location = "my_base_location";
-        let valid_version = 0;
-        let invalid_version = -1;
-
-        let valid_result = create_metadata_location(location, valid_version)?;
-        let invalid_result = create_metadata_location(location, invalid_version);
-
-        assert!(valid_result.starts_with("my_base_location/metadata/00000-"));
-        assert!(valid_result.ends_with(".metadata.json"));
-        assert!(invalid_result.is_err());
-
-        Ok(())
-    }
-
     #[test]
     fn test_get_default_table_location() -> Result<()> {
         let properties = HashMap::from([(LOCATION.to_string(), "db_location".to_string())]);

crates/catalog/hms/src/utils.rs

@@ -35,7 +35,6 @@ aws-config = { workspace = true }
 aws-sdk-s3tables = "1.10.0"
 iceberg = { workspace = true }
 typed-builder = { workspace = true }
-uuid = { workspace = true, features = ["v4"] }
 
 [dev-dependencies]
 iceberg_test_utils = { path = "../../test_utils", features = ["tests"] }

crates/catalog/s3tables/Cargo.toml

@@ -28,12 +28,12 @@ use iceberg::io::{FileIO, FileIOBuilder};
 use iceberg::spec::{TableMetadata, TableMetadataBuilder};
 use iceberg::table::Table;
 use iceberg::{
-    Catalog, Error, ErrorKind, Namespace, NamespaceIdent, Result, TableCommit, TableCreation,
-    TableIdent,
+    Catalog, Error, ErrorKind, MetadataLocation, Namespace, NamespaceIdent, Result, TableCommit,
+    TableCreation, TableIdent,
 };
 use typed_builder::TypedBuilder;
 
-use crate::utils::{create_metadata_location, create_sdk_config};
+use crate::utils::create_sdk_config;
 
 /// S3Tables catalog configuration.
 #[derive(Debug, TypedBuilder)]
@@ -325,7 +325,7 @@ impl Catalog for S3TablesCatalog {
                     .await
                     .map_err(from_aws_sdk_error)?;
                 let warehouse_location = get_resp.warehouse_location().to_string();
-                create_metadata_location(warehouse_location, 0)?
+                MetadataLocation::new_with_table_location(warehouse_location).to_string()
             }
         };
 

crates/catalog/s3tables/src/catalog.rs

@@ -19,8 +19,6 @@ use std::collections::HashMap;
 
 use aws_config::{BehaviorVersion, Region, SdkConfig};
 use aws_sdk_s3tables::config::Credentials;
-use iceberg::{Error, ErrorKind, Result};
-use uuid::Uuid;
 
 /// Property aws profile name
 pub const AWS_PROFILE_NAME: &str = "profile_name";
@@ -71,30 +69,3 @@ pub(crate) async fn create_sdk_config(
 
     config.load().await
 }
-
-/// Create metadata location from `location` and `version`
-pub(crate) fn create_metadata_location(
-    warehouse_location: impl AsRef<str>,
-    version: i32,
-) -> Result<String> {
-    if version < 0 {
-        return Err(Error::new(
-            ErrorKind::DataInvalid,
-            format!(
-                "Table metadata version: '{}' must be a non-negative integer",
-                version
-            ),
-        ));
-    };
-
-    let version = format!("{:0>5}", version);
-    let id = Uuid::new_v4();
-    let metadata_location = format!(
-        "{}/metadata/{}-{}.metadata.json",
-        warehouse_location.as_ref(),
-        version,
-        id
-    );
-
-    Ok(metadata_location)
-}