fix(catalog): reject dropping non-empty MemoryCatalog namespaces - #2890
fix(catalog): reject dropping non-empty MemoryCatalog namespaces#2890fallintoplace wants to merge 3 commits into
Conversation
fallintoplace
changed the title
fallintoplace
changed the title
| let table_count = namespace_state.table_metadata_locations.len(); | ||
| if child_namespace_count > 0 || table_count > 0 { | ||
| return Err(Error::new( | ||
| ErrorKind::Unexpected, |
There was a problem hiding this comment.
PreconditionFailed might be a tad more fitting
| ErrorKind::Unexpected, | |
| ErrorKind::PreconditionFailed, |
/// The operation was rejected because the system is not in a state required for the operation’s execution.
There was a problem hiding this comment.
A better option would be to create a new ErrorKind::NamespaceNotEmpty (analogous to the existing ErrorKind::NamespaceAlreadyExists). Equivalents already exists in other languages like pyiceberg's NamespaceNotEmptyError and Java's NamespaceNotEmptyException.
There was a problem hiding this comment.
Thanks. All of these suggestions are valid. I have pushed the fix.
|
|
||
| #[tokio::test] | ||
| async fn test_dropping_a_namespace_also_drops_namespaces_nested_under_that_one() { | ||
| async fn test_drop_namespace_throws_error_if_namespace_has_children() { |
There was a problem hiding this comment.
nit: Rust doesn't really have a concept of throwing
| async fn test_drop_namespace_throws_error_if_namespace_has_children() { | |
| async fn test_drop_namespace_returns_error_if_namespace_has_children() { |
|
You will need to re-generate the public API files (with the new ErrorKind). Otherwise looks good to me 👍 |
Add ErrorKind::NamespaceNotEmpty to the generated iceberg public API snapshot.
6dcf093 to
9aee948
Compare
2 participants
Which issue does this PR close?
N/A. Found while comparing
MemoryCatalognamespace behavior with the Java reference implementation and other catalog implementations.What changes are included in this PR?
MemoryCatalogpreviously removed a namespace state directly. Because that state owns its child namespaces and table registrations, dropping a non-empty namespace silently discarded the subtree and made registered tables unreachable.This PR checks for child namespaces and registered tables before removal while holding the same catalog mutation lock. A non-empty namespace now returns an error that reports the remaining child and table counts, and no catalog state is modified.
Are these changes tested?
Yes. Unit tests verify that drops are rejected for namespaces containing child namespaces or tables, and that the rejected operation preserves the namespace, its children, and its tables.
Validated with:
cargo test -p iceberg --libcargo clippy -p iceberg --lib -- -D warnings