fix(catalog): reject dropping non-empty MemoryCatalog namespaces - #2890
Open
fallintoplace wants to merge 2 commits into
Open
fix(catalog): reject dropping non-empty MemoryCatalog namespaces#2890fallintoplace wants to merge 2 commits into
fallintoplace wants to merge 2 commits into
Conversation
fallintoplace
changed the title
fallintoplace
changed the title
DerGut
reviewed
Jul 28, 2026
| let table_count = namespace_state.table_metadata_locations.len(); | ||
| if child_namespace_count > 0 || table_count > 0 { | ||
| return Err(Error::new( | ||
| ErrorKind::Unexpected, |
Contributor
There was a problem hiding this comment.
PreconditionFailed might be a tad more fitting
Suggested change
| ErrorKind::Unexpected, | |
| ErrorKind::PreconditionFailed, |
/// The operation was rejected because the system is not in a state required for the operation’s execution.
Contributor
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.
Contributor
Author
There was a problem hiding this comment.
Thanks. All of these suggestions are valid. I have pushed the fix.
DerGut
reviewed
Jul 28, 2026
|
|
||
| #[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() { |
Contributor
There was a problem hiding this comment.
nit: Rust doesn't really have a concept of throwing
Suggested change
| async fn test_drop_namespace_throws_error_if_namespace_has_children() { | |
| async fn test_drop_namespace_returns_error_if_namespace_has_children() { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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