laskoviymishka commented on code in PR #3101:
URL: https://github.com/apache/iceberg-rust/pull/3101#discussion_r3890693176
##########
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:
The PR body says it preserves warehouse validation, but this quietly moves
it from `load()` to first table creation. `load("memory", HashMap::new())` used
to return an error immediately; now `from_properties` is happy with a missing
key (it defaults to `""`), `MemoryCatalog::new` has no warehouse check, and the
catalog builds fine. The error only surfaces later in `create_table`, and only
when the target namespace has no `location` (the `None if
self.properties.warehouse.is_empty()` arm at line 306) — so a catalog with no
warehouse whose namespaces all carry a `location` never errors at all.
That's a fail-fast regression, and it also diverges from the Rust siblings:
Glue and HMS both reject an empty warehouse at `load()`. I'd move the non-empty
check so it runs before we hand back a catalog — either inside
`MemoryCatalog::new`, or by making the property genuinely required (see my note
on the annotation). wdyt?
##########
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:
`default = ""` combined with `parse_with = parse_warehouse` is incoherent:
the macro expands to `match get(key) { Some(v) => parse_warehouse(v)?, None =>
"".into() }`, so `parse_warehouse` only ever runs on an explicitly-present
value. Absent key gives `""` with no error; explicit `warehouse = ""` errors.
Same end state, two behaviors — and
`test_catalog_properties_with_default_warehouse` locks the asymmetry in as
correct.
If warehouse is required, drop `default` and let the missing key error
through `parse_warehouse`. If an empty warehouse is genuinely allowed (matching
Java's permissive `InMemoryCatalog`), make it `Option<String>` with `default =
None` and handle `None` at the point of use. Either's fine, but the current
middle ground validates one path and silently accepts the other.
##########
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:
Two things on this rewrite. It asserts on `error.to_string()`, which couples
the test to display formatting — `test_catalog_properties` right above uses
`error.kind()` + `error.message()`, which is the pattern elsewhere in this file
and won't break under a formatting refactor.
More importantly, the name still says "throws error" but the error now fires
inside `create_table` rather than at `load` — the contract changed and the
rewrite hides that. If the deferred behavior is intended, I'd add a companion
`test_load_succeeds_without_warehouse` asserting `load` returns `Ok`, so both
sides of the new contract are explicit.
##########
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:
Making `MemoryCatalogProperties` `pub` adds it to the public API, and it's
the first catalog config struct we've exported — `RestCatalogConfig` and
`GlueCatalogConfig` are both `pub(crate)`. On top of that, `warehouse` has no
getter, so the only thing an external caller can do is call `from_properties`
and get back a value they can't read. That's surface area with no capability,
and it's a stability commitment we'd have to honor — the signature shifts the
moment `warehouse` becomes `Option<String>`.
I'd make it `pub(crate)` to match the other catalogs;
`MEMORY_CATALOG_WAREHOUSE` can stay `pub`. Once it's out of the snapshot, easy
to revisit later if we ever want it exported deliberately.
##########
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:
Small thing: this reads as "a warehouse defaulted to some location," but
it's really asserting that an absent key leaves `warehouse` empty.
`test_catalog_properties_warehouse_defaults_to_empty` would match what it
checks.
##########
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:
The PR description mentions coverage for retained catalog properties, but
this test passes `custom.property` in and never asserts anything about it — it
only checks `properties.warehouse`. So nothing here verifies a custom property
actually survives into FileIO or the KMS factory, which is exactly the path
that changed (see the `with_props` line above). If someone later strips extra
keys during forwarding, no test catches it.
I'd add an assertion that a custom property reaches the downstream consumer
— build a `MemoryCatalog` with one and inspect `file_io`'s config, or use a
tracking `KmsClientFactory`. wdyt?
##########
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:
Tiny one: `warehouse.to_owned()` reads a touch more directly than
`.to_string()` for `&str -> String` here.
##########
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:
We're now forwarding the raw `props` (including the `warehouse` key)
straight into `FileIOBuilder::with_props` here, and into `create_kms_client` at
line 83. The old code filtered `MEMORY_CATALOG_WAREHOUSE` out first.
`warehouse` is a modeled catalog property, not a FileIO/KMS key — Glue
explicitly strips its catalog-specific keys before building FileIO, and a
custom `StorageFactory` or `KmsClientFactory` (both public extension points)
will now see the spurious entry.
I'd filter it once in `load` before forwarding to both consumers:
```rust
let catalog_props: HashMap<String, String> = props
.into_iter()
.filter(|(k, _)| k != MEMORY_CATALOG_WAREHOUSE)
.collect();
```
--
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]
