blackmwk commented on code in PR #3101:
URL: https://github.com/apache/iceberg-rust/pull/3101#discussion_r3880965866
##########
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:
This could be moved to warehouse's parse function.
##########
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:
It's not file_io only, we should keep the raw HashMap in MemoryCatalog.
##########
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:
We don't need to do the removal.
--
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]
