blackmwk commented on code in PR #3101:
URL: https://github.com/apache/iceberg-rust/pull/3101#discussion_r3885174386
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -85,69 +70,80 @@ impl CatalogBuilder for MemoryCatalogBuilder {
}
fn load(
- mut self,
+ self,
name: impl Into<String>,
props: HashMap<String, String>,
) -> impl Future<Output = Result<Self::C>> + Send {
- self.config.name = Some(name.into());
-
- if props.contains_key(MEMORY_CATALOG_WAREHOUSE) {
- self.config.warehouse = props
- .get(MEMORY_CATALOG_WAREHOUSE)
- .cloned()
- .unwrap_or_default()
- }
-
- // Collect other remaining properties
- self.config.props = props
- .into_iter()
- .filter(|(k, _)| k != MEMORY_CATALOG_WAREHOUSE)
- .collect();
+ let name = name.into();
async move {
- if self.config.name.is_none() {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name is required",
- ))
- } else if self.config.warehouse.is_empty() {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog warehouse is required",
- ))
- } else {
- let runtime = self.runtime.unwrap_or_else(Runtime::current);
- let kms_client = match self.kms_client_factory {
- Some(factory) =>
Some(factory.create_kms_client(&self.config.props).await?),
- None => None,
- };
- MemoryCatalog::new(self.config, self.storage_factory, runtime,
kms_client)
- }
+ let catalog_properties =
MemoryCatalogProperties::from_properties(&props)?;
+ let runtime = self.runtime.unwrap_or_else(Runtime::current);
+ let kms_client = match self.kms_client_factory {
+ Some(factory) =>
Some(factory.create_kms_client(&props).await?),
+ None => None,
+ };
+ MemoryCatalog::new(
+ name,
+ catalog_properties,
+ props,
+ self.storage_factory,
+ runtime,
+ kms_client,
+ )
}
}
}
-#[derive(Clone, Debug)]
-pub(crate) struct MemoryCatalogConfig {
- name: Option<String>,
+fn parse_warehouse(warehouse: &str) -> Result<String> {
+ if warehouse.is_empty() {
+ Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Catalog warehouse is required",
+ ))
+ } else {
+ Ok(warehouse.to_string())
+ }
+}
+
+/// Memory catalog properties parsed from a catalog property map.
+#[derive(Debug, Properties)]
+pub struct MemoryCatalogProperties {
+ #[property(
+ key = MEMORY_CATALOG_WAREHOUSE,
+ default = parse_warehouse("")?,
Review Comment:
```suggestion
default = "",
```
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -458,6 +462,72 @@ pub(crate) mod tests {
temp_dir.path().to_str().unwrap().to_string()
}
+ #[test]
+ fn test_catalog_properties() {
+ let properties =
MemoryCatalogProperties::from_properties(&HashMap::from([
+ (
+ MEMORY_CATALOG_WAREHOUSE.to_string(),
+ "memory:///warehouse".to_string(),
+ ),
+ ("custom.property".to_string(), "value".to_string()),
+ ]))
+ .unwrap();
+
+ assert_eq!(properties.warehouse, "memory:///warehouse");
+
+ let error =
MemoryCatalogProperties::from_properties(&HashMap::new()).unwrap_err();
+ assert_eq!(error.kind(), ErrorKind::DataInvalid);
+ assert_eq!(error.message(), "Catalog warehouse is required");
+
+ let error = MemoryCatalogProperties::from_properties(&HashMap::from([(
+ MEMORY_CATALOG_WAREHOUSE.to_string(),
+ String::new(),
+ )]))
+ .unwrap_err();
+ assert_eq!(error.kind(), ErrorKind::DataInvalid);
+ assert_eq!(error.message(), "Catalog warehouse is required");
+ }
+
+ #[tokio::test]
+ async fn test_catalog_retains_name_and_properties() {
Review Comment:
Useless, remove it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
