blackmwk commented on code in PR #3101:
URL: https://github.com/apache/iceberg-rust/pull/3101#discussion_r3881018551
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -85,69 +70,72 @@ 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() {
+ let catalog_properties =
MemoryCatalogProperties::from_properties(&props)?;
+ if catalog_properties.warehouse.is_empty() {
Review Comment:
Moved the required-value validation into `parse_warehouse`; both a missing
property and an explicitly empty value now go through it. Added coverage for
both cases in 301ead28.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -85,69 +70,72 @@ 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() {
+ let catalog_properties =
MemoryCatalogProperties::from_properties(&props)?;
+ if catalog_properties.warehouse.is_empty() {
Err(Error::new(
ErrorKind::DataInvalid,
"Catalog warehouse is required",
))
} else {
+ let mut remaining_props = props;
+ remaining_props.remove(MEMORY_CATALOG_WAREHOUSE);
Review Comment:
Removed the warehouse-key removal. The original property map now flows
unchanged to the KMS factory and catalog in 301ead28.
##########
crates/iceberg/src/catalog/memory/catalog.rs:
##########
@@ -85,69 +70,72 @@ 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() {
+ let catalog_properties =
MemoryCatalogProperties::from_properties(&props)?;
+ if catalog_properties.warehouse.is_empty() {
Err(Error::new(
ErrorKind::DataInvalid,
"Catalog warehouse is required",
))
} else {
+ let mut remaining_props = props;
+ remaining_props.remove(MEMORY_CATALOG_WAREHOUSE);
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?),
+ Some(factory) =>
Some(factory.create_kms_client(&remaining_props).await?),
None => None,
};
- MemoryCatalog::new(self.config, self.storage_factory, runtime,
kms_client)
+ MemoryCatalog::new(
+ name,
+ catalog_properties,
+ remaining_props,
+ self.storage_factory,
+ runtime,
+ kms_client,
+ )
}
}
}
}
-#[derive(Clone, Debug)]
-pub(crate) struct MemoryCatalogConfig {
- name: Option<String>,
+/// Memory catalog properties parsed from a catalog property map.
+#[derive(Debug, Properties)]
+pub struct MemoryCatalogProperties {
+ #[property(key = MEMORY_CATALOG_WAREHOUSE, default = "")]
warehouse: String,
- props: HashMap<String, String>,
}
/// Memory catalog implementation.
-#[derive(Debug)]
pub struct MemoryCatalog {
+ name: String,
+ properties: MemoryCatalogProperties,
root_namespace_state: Mutex<NamespaceState>,
file_io: FileIO,
- warehouse_location: String,
runtime: Runtime,
kms_client: Option<Arc<dyn KeyManagementClient>>,
}
+impl std::fmt::Debug for MemoryCatalog {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("MemoryCatalog")
+ .field("name", &self.name)
+ .field("properties", &self.properties)
+ .finish_non_exhaustive()
+ }
+}
+
impl MemoryCatalog {
/// Creates a memory catalog.
fn new(
- config: MemoryCatalogConfig,
+ name: String,
+ properties: MemoryCatalogProperties,
+ file_io_props: HashMap<String, String>,
Review Comment:
Renamed the constructor input to `props`, retained that raw `HashMap` on
`MemoryCatalog`, and clone it into `FileIO` in 301ead28.
--
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]
