liurenjie1024 commented on code in PR #237:
URL: https://github.com/apache/iceberg-rust/pull/237#discussion_r1518487095
##########
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(
+ namespace: &NamespaceIdent,
+ properties: &HashMap<String, String>,
+ ) -> Result<Database> {
+ let name = HmsCatalog::validate_namespace(namespace)?;
+ HmsCatalog::validate_owner_settings(properties)?;
+
+ let mut db = Database::default();
+ let mut parameters = AHashMap::new();
+
+ db.name = Some(name.into());
+
+ for (k, v) in properties {
+ match k.as_str() {
+ "comment" => db.description = Some(v.clone().into()),
+ "location" => {
+ db.location_uri =
Some(HmsCatalog::format_location_uri(v.clone()).into())
+ }
+ HMS_DB_OWNER => db.owner_name = Some(v.clone().into()),
+ HMS_DB_OWNER_TYPE => {
+ let owner_type = match v.to_lowercase().as_str() {
+ "user" => PrincipalType::User,
+ "group" => PrincipalType::Group,
+ "role" => PrincipalType::Role,
+ _ => {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!("Invalid value for setting
'owner_type': {}", v),
+ ))
+ }
+ };
+ db.owner_type = Some(owner_type);
+ }
+ _ => {
+ parameters.insert(
+ FastStr::from_string(k.clone()),
+ FastStr::from_string(v.clone()),
+ );
+ }
+ }
+ }
+
+ db.parameters = Some(parameters);
+
+ // Set default user, if none provided
+ //
https://github.com/apache/iceberg/blob/main/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveHadoopUtil.java#L44
+ if db.owner_name.is_none() {
+ db.owner_name = Some("user.name".into());
+ db.owner_type = Some(PrincipalType::User);
+ }
+
+ Ok(db)
+ }
+
+ /// Checks if provided `NamespaceIdent` is valid.
+ pub fn validate_namespace(namespace: &NamespaceIdent) -> Result<String> {
+ let name = namespace.as_ref();
+
+ if name.len() != 1 {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Invalid database, hierarchical namespaces are not supported",
Review Comment:
```suggestion
format!("Invalid database name: {namespace}, hierarchical
namespaces are not supported"),
```
##########
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> {
Review Comment:
+1, also I'm thinking about putting all these properties such as "comment",
"location" in to constants.
##########
crates/catalog/hms/testdata/hms_catalog/docker-compose.yaml:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+version: '3.8'
+
+services:
+ minio:
+ image: minio/minio
Review Comment:
Please add a version so that the result is always reproducible.
##########
crates/catalog/hms/Cargo.toml:
##########
@@ -33,5 +33,12 @@ anyhow = { workspace = true }
async-trait = { workspace = true }
hive_metastore = { workspace = true }
iceberg = { workspace = true }
+log = "0.4.20"
+pilota = "0.10.0"
Review Comment:
Ditto
##########
crates/catalog/hms/testdata/hms_catalog/Dockerfile:
##########
Review Comment:
I'm not sure if we should maintain this docker file, there already one here:
https://github.com/naushadh/hive-metastore
I tried it and it works will for me, see [this
example](https://github.com/risingwavelabs/risingwave/blob/73e6e0b49e60c9cbfc11352c01a1c51305888e49/integration_tests/iceberg-sink2/docker/hive/docker-compose.yml#L85)
cc @Xuanwo @Fokko What do you think?
##########
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:
How about we expose only two methods to convert between `Database` and
`Namespace`, rather than `HashMap<String, String>` so that they these two apis
are more consistent. We can have a new type such as following:
```rust
pub struct HmsDatabase(Database);
```
--
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
