From beb29b4e0ef3b87f9ba37ae346f0a931b37d6502 Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:12:30 +0800 Subject: [PATCH 1/5] refactor(memory): derive catalog configuration from properties Replace manual catalog property extraction with the shared derive while preserving warehouse validation and forwarding unmodeled properties. Tracks [apache/iceberg-rust#3095](https://github.com/apache/iceberg-rust/issues/3095). Generated-by: Codex --- crates/iceberg/src/catalog/memory/catalog.rs | 46 ++++++++++++++------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index 85fcd824ef..6a522b8fd6 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -23,6 +23,7 @@ use std::sync::Arc; use async_trait::async_trait; use futures::lock::{Mutex, MutexGuard}; +use iceberg_property_macro::Properties; use itertools::Itertools; use super::namespace_state::NamespaceState; @@ -91,20 +92,12 @@ impl CatalogBuilder for MemoryCatalogBuilder { ) -> impl Future> + 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(); - async move { + let mut catalog_properties = MemoryCatalogProperties::from_properties(&props)?; + catalog_properties.props.remove(MEMORY_CATALOG_WAREHOUSE); + self.config.warehouse = catalog_properties.warehouse; + self.config.props = catalog_properties.props; + if self.config.name.is_none() { Err(Error::new( ErrorKind::DataInvalid, @@ -127,6 +120,14 @@ impl CatalogBuilder for MemoryCatalogBuilder { } } +#[derive(Properties)] +struct MemoryCatalogProperties { + #[property(key = MEMORY_CATALOG_WAREHOUSE, default = "")] + warehouse: String, + #[property(prefix = "")] + props: HashMap, +} + #[derive(Clone, Debug)] pub(crate) struct MemoryCatalogConfig { name: Option, @@ -458,6 +459,25 @@ 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"); + assert_eq!( + properties.props[MEMORY_CATALOG_WAREHOUSE], + "memory:///warehouse" + ); + assert_eq!(properties.props["custom.property"], "value"); + } + pub(crate) async fn new_memory_catalog() -> impl Catalog { let warehouse_location = temp_path(); MemoryCatalogBuilder::default() From 6b8a0037a9058e75388f30ed3de1400c07f404e1 Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:18:06 +0800 Subject: [PATCH 2/5] refactor(memory): retain typed catalog properties Remove the redundant config wrapper and catch-all property field, expose the typed properties, and retain the catalog name and typed properties on MemoryCatalog. Addresses review feedback on [apache/iceberg-rust#3101](https://github.com/apache/iceberg-rust/pull/3101). Generated-by: Codex --- crates/iceberg/public-api.txt | 7 +- crates/iceberg/src/catalog/memory/catalog.rs | 124 +++++++++++-------- 2 files changed, 78 insertions(+), 53 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 4aeddb665a..0dec073bff 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -1112,7 +1112,7 @@ pub fn iceberg::memory::MemoryCatalog::update_namespace<'life0, 'life1, 'async_t pub fn iceberg::memory::MemoryCatalog::update_table<'life0, 'async_trait>(&'life0 self, commit: iceberg::TableCommit) -> core::pin::Pin> + core::marker::Send + 'async_trait)>> where Self: 'async_trait, 'life0: 'async_trait pub struct iceberg::memory::MemoryCatalogBuilder impl core::default::Default for iceberg::memory::MemoryCatalogBuilder -pub fn iceberg::memory::MemoryCatalogBuilder::default() -> Self +pub fn iceberg::memory::MemoryCatalogBuilder::default() -> iceberg::memory::MemoryCatalogBuilder impl core::fmt::Debug for iceberg::memory::MemoryCatalogBuilder pub fn iceberg::memory::MemoryCatalogBuilder::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl iceberg::CatalogBuilder for iceberg::memory::MemoryCatalogBuilder @@ -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) -> 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) -> Self +pub struct iceberg::memory::MemoryCatalogProperties +impl iceberg::memory::MemoryCatalogProperties +pub fn iceberg::memory::MemoryCatalogProperties::from_properties(properties: &std::collections::hash::map::HashMap) -> iceberg::Result +impl core::fmt::Debug for iceberg::memory::MemoryCatalogProperties +pub fn iceberg::memory::MemoryCatalogProperties::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result pub const iceberg::memory::MEMORY_CATALOG_WAREHOUSE: &str pub mod iceberg::metadata_columns pub const iceberg::metadata_columns::RESERVED_COL_NAME_CHANGE_ORDINAL: &str diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index 6a522b8fd6..8b3c5b6c01 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -44,29 +44,13 @@ pub const MEMORY_CATALOG_WAREHOUSE: &str = "warehouse"; const LOCATION: &str = "location"; /// Builder for [`MemoryCatalog`]. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct MemoryCatalogBuilder { - config: MemoryCatalogConfig, storage_factory: Option>, kms_client_factory: Option>, runtime: Option, } -impl Default for MemoryCatalogBuilder { - fn default() -> Self { - Self { - config: MemoryCatalogConfig { - name: None, - warehouse: "".to_string(), - props: HashMap::new(), - }, - storage_factory: None, - kms_client_factory: None, - runtime: None, - } - } -} - impl CatalogBuilder for MemoryCatalogBuilder { type C = MemoryCatalog; @@ -86,69 +70,72 @@ impl CatalogBuilder for MemoryCatalogBuilder { } fn load( - mut self, + self, name: impl Into, props: HashMap, ) -> impl Future> + Send { - self.config.name = Some(name.into()); + let name = name.into(); async move { - let mut catalog_properties = MemoryCatalogProperties::from_properties(&props)?; - catalog_properties.props.remove(MEMORY_CATALOG_WAREHOUSE); - self.config.warehouse = catalog_properties.warehouse; - self.config.props = catalog_properties.props; - - 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(Properties)] -struct MemoryCatalogProperties { +/// Memory catalog properties parsed from a catalog property map. +#[derive(Debug, Properties)] +pub struct MemoryCatalogProperties { #[property(key = MEMORY_CATALOG_WAREHOUSE, default = "")] warehouse: String, - #[property(prefix = "")] - props: HashMap, -} - -#[derive(Clone, Debug)] -pub(crate) struct MemoryCatalogConfig { - name: Option, - warehouse: String, - props: HashMap, } /// Memory catalog implementation. -#[derive(Debug)] pub struct MemoryCatalog { + name: String, + properties: MemoryCatalogProperties, root_namespace_state: Mutex, file_io: FileIO, - warehouse_location: String, runtime: Runtime, kms_client: Option>, } +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, storage_factory: Option>, runtime: Runtime, kms_client: Option>, @@ -157,9 +144,12 @@ impl MemoryCatalog { let factory = storage_factory.unwrap_or_else(|| Arc::new(MemoryStorageFactory)); Ok(Self { + name, + properties, root_namespace_state: Mutex::new(NamespaceState::default()), - file_io: FileIOBuilder::new(factory).with_props(config.props).build(), - warehouse_location: config.warehouse, + file_io: FileIOBuilder::new(factory) + .with_props(file_io_props) + .build(), runtime, kms_client, }) @@ -309,7 +299,11 @@ impl Catalog for MemoryCatalog { let namespace_properties = root_namespace_state.get_properties(namespace_ident)?; let location_prefix = match namespace_properties.get(LOCATION) { Some(namespace_location) => namespace_location.clone(), - None => format!("{}/{}", self.warehouse_location, namespace_ident.join("/")), + None => format!( + "{}/{}", + self.properties.warehouse, + namespace_ident.join("/") + ), }; let location = format!("{}/{}", location_prefix, table_ident.name()); @@ -471,11 +465,37 @@ pub(crate) mod tests { .unwrap(); assert_eq!(properties.warehouse, "memory:///warehouse"); + } + + #[tokio::test] + async fn test_catalog_retains_name_and_properties() { + let catalog = MemoryCatalogBuilder::default() + .load( + "memory", + HashMap::from([ + ( + MEMORY_CATALOG_WAREHOUSE.to_string(), + "memory:///warehouse".to_string(), + ), + ("custom.property".to_string(), "value".to_string()), + ]), + ) + .await + .unwrap(); + + assert_eq!(catalog.name, "memory"); + assert_eq!(catalog.properties.warehouse, "memory:///warehouse"); assert_eq!( - properties.props[MEMORY_CATALOG_WAREHOUSE], - "memory:///warehouse" + catalog.file_io.config().props().get("custom.property"), + Some(&"value".to_string()) + ); + assert!( + !catalog + .file_io + .config() + .props() + .contains_key(MEMORY_CATALOG_WAREHOUSE) ); - assert_eq!(properties.props["custom.property"], "value"); } pub(crate) async fn new_memory_catalog() -> impl Catalog { From 301ead28509bb6122ffbc6dba74a7483673572a1 Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Fri, 28 Aug 2026 21:35:59 +0800 Subject: [PATCH 3/5] refactor(memory): retain raw catalog properties --- crates/iceberg/src/catalog/memory/catalog.rs | 88 +++++++++++++------- 1 file changed, 59 insertions(+), 29 deletions(-) diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index 8b3c5b6c01..8c82176443 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -78,36 +78,42 @@ impl CatalogBuilder for MemoryCatalogBuilder { async move { 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(&remaining_props).await?), - None => None, - }; - MemoryCatalog::new( - name, - catalog_properties, - remaining_props, - self.storage_factory, - runtime, - kms_client, - ) - } + 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, + ) } } } +fn parse_warehouse(warehouse: &str) -> Result { + 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 = "")] + #[property( + key = MEMORY_CATALOG_WAREHOUSE, + default = parse_warehouse("")?, + parse_with = parse_warehouse + )] warehouse: String, } @@ -115,6 +121,7 @@ pub struct MemoryCatalogProperties { pub struct MemoryCatalog { name: String, properties: MemoryCatalogProperties, + props: HashMap, root_namespace_state: Mutex, file_io: FileIO, runtime: Runtime, @@ -126,6 +133,7 @@ impl std::fmt::Debug for MemoryCatalog { f.debug_struct("MemoryCatalog") .field("name", &self.name) .field("properties", &self.properties) + .field("props", &self.props) .finish_non_exhaustive() } } @@ -135,7 +143,7 @@ impl MemoryCatalog { fn new( name: String, properties: MemoryCatalogProperties, - file_io_props: HashMap, + props: HashMap, storage_factory: Option>, runtime: Runtime, kms_client: Option>, @@ -146,10 +154,11 @@ impl MemoryCatalog { Ok(Self { name, properties, - root_namespace_state: Mutex::new(NamespaceState::default()), file_io: FileIOBuilder::new(factory) - .with_props(file_io_props) + .with_props(props.clone()) .build(), + props, + root_namespace_state: Mutex::new(NamespaceState::default()), runtime, kms_client, }) @@ -465,6 +474,18 @@ pub(crate) mod tests { .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] @@ -485,16 +506,25 @@ pub(crate) mod tests { assert_eq!(catalog.name, "memory"); assert_eq!(catalog.properties.warehouse, "memory:///warehouse"); + assert_eq!( + catalog.props.get(MEMORY_CATALOG_WAREHOUSE), + Some(&"memory:///warehouse".to_string()) + ); + assert_eq!( + catalog.props.get("custom.property"), + Some(&"value".to_string()) + ); assert_eq!( catalog.file_io.config().props().get("custom.property"), Some(&"value".to_string()) ); - assert!( - !catalog + assert_eq!( + catalog .file_io .config() .props() - .contains_key(MEMORY_CATALOG_WAREHOUSE) + .get(MEMORY_CATALOG_WAREHOUSE), + Some(&"memory:///warehouse".to_string()) ); } From 954c387f5814b23e7bc1bc2195e0dbf967651a92 Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Sat, 29 Aug 2026 10:28:48 +0800 Subject: [PATCH 4/5] fix(memory): defer missing warehouse validation --- crates/iceberg/src/catalog/memory/catalog.rs | 74 +++++++------------- 1 file changed, 27 insertions(+), 47 deletions(-) diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index 8c82176443..539b925261 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -111,7 +111,7 @@ fn parse_warehouse(warehouse: &str) -> Result { pub struct MemoryCatalogProperties { #[property( key = MEMORY_CATALOG_WAREHOUSE, - default = parse_warehouse("")?, + default = "", parse_with = parse_warehouse )] warehouse: String, @@ -308,6 +308,12 @@ impl Catalog for MemoryCatalog { let namespace_properties = root_namespace_state.get_properties(namespace_ident)?; let location_prefix = match namespace_properties.get(LOCATION) { Some(namespace_location) => namespace_location.clone(), + None if self.properties.warehouse.is_empty() => { + return Err(Error::new( + ErrorKind::DataInvalid, + "Catalog warehouse is required", + )); + } None => format!( "{}/{}", self.properties.warehouse, @@ -475,9 +481,8 @@ pub(crate) mod tests { 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 properties = MemoryCatalogProperties::from_properties(&HashMap::new()).unwrap(); + assert!(properties.warehouse.is_empty()); let error = MemoryCatalogProperties::from_properties(&HashMap::from([( MEMORY_CATALOG_WAREHOUSE.to_string(), @@ -488,46 +493,6 @@ pub(crate) mod tests { assert_eq!(error.message(), "Catalog warehouse is required"); } - #[tokio::test] - async fn test_catalog_retains_name_and_properties() { - let catalog = MemoryCatalogBuilder::default() - .load( - "memory", - HashMap::from([ - ( - MEMORY_CATALOG_WAREHOUSE.to_string(), - "memory:///warehouse".to_string(), - ), - ("custom.property".to_string(), "value".to_string()), - ]), - ) - .await - .unwrap(); - - assert_eq!(catalog.name, "memory"); - assert_eq!(catalog.properties.warehouse, "memory:///warehouse"); - assert_eq!( - catalog.props.get(MEMORY_CATALOG_WAREHOUSE), - Some(&"memory:///warehouse".to_string()) - ); - assert_eq!( - catalog.props.get("custom.property"), - Some(&"value".to_string()) - ); - assert_eq!( - catalog.file_io.config().props().get("custom.property"), - Some(&"value".to_string()) - ); - assert_eq!( - catalog - .file_io - .config() - .props() - .get(MEMORY_CATALOG_WAREHOUSE), - Some(&"memory:///warehouse".to_string()) - ); - } - pub(crate) async fn new_memory_catalog() -> impl Catalog { let warehouse_location = temp_path(); MemoryCatalogBuilder::default() @@ -1475,11 +1440,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(), "DataInvalid => Catalog warehouse is required" ); } From 994b5956af5c5bedac380feaa80ebd34c12ab118 Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Sun, 30 Aug 2026 09:08:00 +0800 Subject: [PATCH 5/5] refactor(memory): move raw properties into file io --- crates/iceberg/src/catalog/memory/catalog.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/crates/iceberg/src/catalog/memory/catalog.rs b/crates/iceberg/src/catalog/memory/catalog.rs index 539b925261..c3251f2dd7 100644 --- a/crates/iceberg/src/catalog/memory/catalog.rs +++ b/crates/iceberg/src/catalog/memory/catalog.rs @@ -121,7 +121,6 @@ pub struct MemoryCatalogProperties { pub struct MemoryCatalog { name: String, properties: MemoryCatalogProperties, - props: HashMap, root_namespace_state: Mutex, file_io: FileIO, runtime: Runtime, @@ -133,7 +132,6 @@ impl std::fmt::Debug for MemoryCatalog { f.debug_struct("MemoryCatalog") .field("name", &self.name) .field("properties", &self.properties) - .field("props", &self.props) .finish_non_exhaustive() } } @@ -154,10 +152,7 @@ impl MemoryCatalog { Ok(Self { name, properties, - file_io: FileIOBuilder::new(factory) - .with_props(props.clone()) - .build(), - props, + file_io: FileIOBuilder::new(factory).with_props(props).build(), root_namespace_state: Mutex::new(NamespaceState::default()), runtime, kms_client, @@ -481,9 +476,6 @@ pub(crate) mod tests { assert_eq!(properties.warehouse, "memory:///warehouse"); - let properties = MemoryCatalogProperties::from_properties(&HashMap::new()).unwrap(); - assert!(properties.warehouse.is_empty()); - let error = MemoryCatalogProperties::from_properties(&HashMap::from([( MEMORY_CATALOG_WAREHOUSE.to_string(), String::new(), @@ -493,6 +485,13 @@ pub(crate) mod tests { assert_eq!(error.message(), "Catalog warehouse is required"); } + #[test] + fn test_catalog_properties_with_default_warehouse() { + let properties = MemoryCatalogProperties::from_properties(&HashMap::new()).unwrap(); + + assert_eq!(properties.warehouse, ""); + } + pub(crate) async fn new_memory_catalog() -> impl Catalog { let warehouse_location = temp_path(); MemoryCatalogBuilder::default()