marvinlanhenke commented on code in PR #237:
URL: https://github.com/apache/iceberg-rust/pull/237#discussion_r1518568918


##########
crates/catalog/hms/src/catalog.rs:
##########
@@ -95,9 +106,156 @@ impl HmsCatalog {
             client: HmsClient(client),
         })
     }
+
+    /// Create and extract properties from `hive_metastore::hms::Database`.
+    pub fn properties_from_database(database: &Database) -> HashMap<String, 
String> {
+        let mut properties = HashMap::new();
+
+        if let Some(description) = &database.description {
+            properties.insert("comment".to_string(), description.to_string());
+        };
+
+        if let Some(location) = &database.location_uri {
+            properties.insert("location".to_string(), location.to_string());
+        };
+
+        if let Some(owner) = &database.owner_name {
+            properties.insert(HMS_DB_OWNER.to_string(), owner.to_string());
+        };
+
+        if let Some(owner_type) = &database.owner_type {
+            let value = match owner_type {
+                PrincipalType::User => "User",
+                PrincipalType::Group => "Group",
+                PrincipalType::Role => "Role",
+            };
+
+            properties.insert(HMS_DB_OWNER_TYPE.to_string(), 
value.to_string());
+        };
+
+        if let Some(params) = &database.parameters {
+            params.iter().for_each(|(k, v)| {
+                properties.insert(k.clone().into(), v.clone().into());
+            });
+        };
+
+        properties
+    }
+
+    /// Converts name and properties into `hive_metastore::hms::Database`
+    /// after validating the `namespace` and `owner-settings`.
+    pub fn convert_to_database(

Review Comment:
   ...in the python and java implementation similiar functions exists simply as 
private / static helper functions inside the respective `Catalog` classes. I 
think it would be fine if we do this as well (at least for now).
   
   However, I'd still change the properties_from_database function to return a 
`Namespace` instead of a HashMap<String, String> which makes way more sense, 
like you suggested.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to