blackmwk commented on code in PR #3101:
URL: https://github.com/apache/iceberg-rust/pull/3101#discussion_r3891235309
##########
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)?;
Review Comment:
Restored fail-fast validation in `load()` in 7ffa23a1. Both missing and
explicitly empty warehouses now return `DataInvalid` before KMS or catalog
construction, with focused tests for both cases.
##########
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())
+ }
+}
+
+/// Memory catalog properties parsed from a catalog property map.
+#[derive(Debug, Properties)]
+pub struct MemoryCatalogProperties {
+ #[property(
Review Comment:
Removed `parse_with` and the custom parser in 7ffa23a1. Missing and
explicitly empty values now parse consistently to the empty default, then share
the same fail-fast validation in `load()`.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -156,9 +150,10 @@ impl MemoryCatalog {
let factory = storage_factory.unwrap_or_else(||
Arc::new(MemoryStorageFactory));
Ok(Self {
+ name,
+ properties,
+ file_io: FileIOBuilder::new(factory).with_props(props).build(),
Review Comment:
The modeled `warehouse` key is now removed once in `load()` before the
remaining map is passed to both KMS and FileIO in 7ffa23a1.
##########
crates/iceberg/public-api.txt:
##########
@@ -1121,6 +1121,11 @@ pub fn iceberg::memory::MemoryCatalogBuilder::load(self,
name: impl core::conver
pub fn iceberg::memory::MemoryCatalogBuilder::with_kms_client_factory(self,
kms_client_factory: alloc::sync::Arc<dyn
iceberg::encryption::kms::KmsClientFactory>) -> Self
pub fn iceberg::memory::MemoryCatalogBuilder::with_runtime(self, runtime:
iceberg::Runtime) -> Self
pub fn iceberg::memory::MemoryCatalogBuilder::with_storage_factory(self,
storage_factory: alloc::sync::Arc<dyn iceberg::io::StorageFactory>) -> Self
+pub struct iceberg::memory::MemoryCatalogProperties
Review Comment:
Changed `MemoryCatalogProperties` to `pub(crate)` and removed it from the
checked-in public API snapshot 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]
