blackmwk commented on code in PR #3101:
URL: https://github.com/apache/iceberg-rust/pull/3101#discussion_r3891235908
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -458,6 +463,35 @@ 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()),
Review Comment:
Added `test_catalog_forwards_custom_properties_to_file_io` in 7ffa23a1. It
verifies that the custom property reaches FileIO and the catalog-only
`warehouse` key does not.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -1405,11 +1439,26 @@ pub(crate) mod tests {
{
let catalog = MemoryCatalogBuilder::default()
.load("memory", HashMap::from([]))
- .await;
+ .await
+ .unwrap();
+ let namespace_ident = NamespaceIdent::new("namespace".into());
+ catalog
+ .create_namespace(&namespace_ident, HashMap::new())
+ .await
+ .unwrap();
+ let error = catalog
+ .create_table(
+ &namespace_ident,
+ TableCreation::builder()
+ .name("table".into())
+ .schema(simple_table_schema())
+ .build(),
+ )
+ .await
+ .unwrap_err();
- assert!(catalog.is_err());
assert_eq!(
- catalog.unwrap_err().to_string(),
+ error.to_string(),
Review Comment:
Restored the original load-time contract in 7ffa23a1. The regression tests
now assert `ErrorKind` and message separately for both missing and explicitly
empty warehouse values.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -458,6 +463,35 @@ 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::from([(
+ MEMORY_CATALOG_WAREHOUSE.to_string(),
+ String::new(),
+ )]))
+ .unwrap_err();
+ assert_eq!(error.kind(), ErrorKind::DataInvalid);
+ assert_eq!(error.message(), "Catalog warehouse is required");
+ }
+
+ #[test]
+ fn test_catalog_properties_with_default_warehouse() {
Review Comment:
Renamed the test to `test_catalog_properties_warehouse_defaults_to_empty`
and made it verify both absent and explicitly empty property parsing in
7ffa23a1.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -85,69 +70,78 @@ 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())
Review Comment:
The custom `parse_warehouse` function is no longer needed after
consolidating validation in `load()`, so it was removed in 7ffa23a1.
--
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]
