fix(catalog): validate namespace existence in InMemoryCatalog create and register - #980
LuciferYang wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
RegisterTable still reads metadata before validating the namespace, allowing the wrong error to surface.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Validates namespaces in InMemoryCatalog table creation and registration to prevent orphan metadata and incorrect errors.
Changes:
- Adds namespace checks to
CreateTableandRegisterTable. - Adds regression tests for missing namespaces and orphan files.
| File | Description |
|---|---|
src/iceberg/catalog/memory/in_memory_catalog.cc |
Adds namespace validation. |
src/iceberg/test/in_memory_catalog_test.cc |
Tests missing-namespace behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
753aec5 to
dd3cf63
Compare
|
Addressed the review note: |
| ICEBERG_ASSIGN_OR_RAISE(auto metadata, | ||
| TableMetadataUtil::Read(*file_io_, metadata_file_location)); |
…and register CreateTable wrote the table metadata file before checking the namespace, leaking an orphaned metadata file on object-store FileIO and returning a misleading kIOError on local FileIO. RegisterTable's if (!NamespaceExists(...)) tested the Result<bool>'s has_value() rather than the contained bool, so the missing-namespace branch was dead code and the error surfaced as kUnknownError. Both paths now unwrap NamespaceExists and return kNoSuchNamespace before any metadata I/O: CreateTable before the write, RegisterTable before the read, so a register into a missing namespace fails fast without a wasted (and possibly failing) metadata read.
dd3cf63 to
ecefc10
Compare
|
Follow-up on the lock note: |
|
On the concurrent-namespace-removal note: the final root_namespace_->RegisterTable re-resolves the namespace under the lock before inserting, so a drop in the window between validation and insert makes the insert fail rather than registering into a missing namespace. The only effect is a less specific error kind in that rare race, and InMemoryCatalog is a not-for-production catalog. |
Read the metadata outside the lock, then validate the namespace and register under a single lock, rather than splitting validation and registration into two critical sections. This keeps the check and insert atomic, so a concurrent namespace drop cannot slip between them, and still never holds the catalog mutex during metadata I/O.
|
Design note for the next review pass: I moved |
| ICEBERG_UNWRAP_OR_FAIL(auto metadata, | ||
| ReadTableMetadataFromResource("TableMetadataV2Valid.json")); | ||
| auto table_location = GenerateTestTableLocation(table_ident.name); | ||
| auto metadata_location = std::format("{}v1.metadata.json", table_location); | ||
| ASSERT_THAT(TableMetadataUtil::Write(*file_io_, metadata_location, *metadata), IsOk()); |
What
InMemoryCatalogdid not correctly check that a table's namespace exists, in two methods.CreateTablewrote the table metadata file before validating the namespace. Creating a table under a missing namespace returnedkNoSuchNamespace, but on object-storeFileIOit first left an orphaned00000-<uuid>.metadata.jsonthat nothing later removes, and on the default localFileIOthe stray write failed with a misleadingkIOError.RegisterTableguarded withif (!root_namespace_->NamespaceExists(identifier.ns)).NamespaceExistsreturnsResult<bool>, so!resulttestshas_value()rather than the bool, and a missing namespace is a valuefalse, not an error. The branch was dead code, and a missing namespace surfaced askUnknownErrorinstead ofkNoSuchNamespace.Closes #977.
How
CreateTablenow validates the namespace before writing any metadata, matchingSqlCatalog::CreateTable.RegisterTablereads the metadata outside the lock, then validates the namespace and registers under a single lock: the deadif (!NamespaceExists(...))check (which tested theResult<bool>'shas_value()rather than the contained bool) becomes an unwrap that returnskNoSuchNamespace. Keeping the namespace check and the registration in one critical section leaves them atomic, and the metadata read stays off the lock. The empty (root) namespace still resolves totrue, so ordinary table creation is unaffected.Testing
Two new tests in
in_memory_catalog_test.cc:CreateTableNonexistentNamespace— creating a table under a missing namespace returnskNoSuchNamespaceand leaves no metadata file behind. It passes an explicit location whosemetadata/directory already exists, so the pre-fix write lands a detectable orphan there; the zero-file assertion is what fails without the fix. The error-kind assertion alone would still pass pre-fix, because the laterUpdateTableMetadataLocationalso returnskNoSuchNamespaceafter the orphan is written.RegisterTableNonexistentNamespace— registering under a missing namespace returnskNoSuchNamespace; without the fix the deadResult<bool>check let it fall through tokUnknownError.