Skip to content

Commit bbc2359

Browse files
authored
fix(catalog): validate namespace existence in InMemoryCatalog create and register (#980)
## What `InMemoryCatalog` did not correctly check that a table's namespace exists, in two methods. `CreateTable` wrote the table metadata file before validating the namespace. Creating a table under a missing namespace returned `kNoSuchNamespace`, but on object-store `FileIO` it first left an orphaned `00000-<uuid>.metadata.json` that nothing later removes, and on the default local `FileIO` the stray write failed with a misleading `kIOError`. `RegisterTable` guarded with `if (!root_namespace_->NamespaceExists(identifier.ns))`. `NamespaceExists` returns `Result<bool>`, so `!result` tests `has_value()` rather than the bool, and a missing namespace is a value `false`, not an error. The branch was dead code, and a missing namespace surfaced as `kUnknownError` instead of `kNoSuchNamespace`. Closes #977. ## How `CreateTable` now validates the namespace before writing any metadata, matching `SqlCatalog::CreateTable`. `RegisterTable` reads the metadata outside the lock, then validates the namespace and registers under a single lock: the dead `if (!NamespaceExists(...))` check (which tested the `Result<bool>`'s `has_value()` rather than the contained bool) becomes an unwrap that returns `kNoSuchNamespace`. 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 to `true`, so ordinary table creation is unaffected. ## Testing Two new tests in `in_memory_catalog_test.cc`: - `CreateTableNonexistentNamespace` — creating a table under a missing namespace returns `kNoSuchNamespace` and leaves no metadata file behind. It passes an explicit location whose `metadata/` 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 later `UpdateTableMetadataLocation` also returns `kNoSuchNamespace` after the orphan is written. - `RegisterTableNonexistentNamespace` — registering under a missing namespace returns `kNoSuchNamespace`; without the fix the dead `Result<bool>` check let it fall through to `kUnknownError`.
1 parent db796cb commit bbc2359

2 files changed

Lines changed: 55 additions & 1 deletion

File tree

‎src/iceberg/catalog/memory/in_memory_catalog.cc‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,11 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::CreateTable(
422422
const std::string& location,
423423
const std::unordered_map<std::string, std::string>& properties) {
424424
std::unique_lock lock(mutex_);
425+
ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists,
426+
root_namespace_->NamespaceExists(identifier.ns));
427+
if (!namespace_exists) {
428+
return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns);
429+
}
425430
if (root_namespace_->TableExists(identifier).value_or(false)) {
426431
return AlreadyExists("Table already exists: {}", identifier);
427432
}
@@ -609,7 +614,9 @@ Result<std::shared_ptr<Table>> InMemoryCatalog::RegisterTable(
609614
TableMetadataUtil::Read(*file_io_, metadata_file_location));
610615

611616
std::unique_lock lock(mutex_);
612-
if (!root_namespace_->NamespaceExists(identifier.ns)) {
617+
ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists,
618+
root_namespace_->NamespaceExists(identifier.ns));
619+
if (!namespace_exists) {
613620
return NoSuchNamespace("Table namespace does not exist: {}", identifier.ns);
614621
}
615622
if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) {

‎src/iceberg/test/in_memory_catalog_test.cc‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,53 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
158158
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
159159
}
160160

161+
TEST_F(InMemoryCatalogTest, CreateTableNonexistentNamespace) {
162+
TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name = "t1"};
163+
auto schema = std::make_shared<Schema>(
164+
std::vector<SchemaField>{SchemaField::MakeRequired(1, "id", int64())},
165+
/*schema_id=*/1);
166+
auto spec = PartitionSpec::Unpartitioned();
167+
auto sort_order = SortOrder::Unsorted();
168+
169+
// Use an explicit location whose metadata directory already exists (the local
170+
// FileIO does not create parent dirs). A write-before-validate bug would land a
171+
// detectable orphan there. GenerateTestTableLocation is unique per test and
172+
// auto-cleaned via created_temp_paths_.
173+
auto table_location = GenerateTestTableLocation(table_ident.name);
174+
175+
auto table =
176+
catalog_->CreateTable(table_ident, schema, spec, sort_order, table_location, {});
177+
EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace));
178+
179+
// The namespace check must run before any metadata file is written, so a
180+
// failed create must leave no orphaned metadata file behind.
181+
std::error_code ec;
182+
size_t metadata_files = 0;
183+
for (auto it = std::filesystem::recursive_directory_iterator(table_location, ec);
184+
it != std::filesystem::recursive_directory_iterator(); it.increment(ec)) {
185+
if (it->path().extension() == ".json") {
186+
++metadata_files;
187+
}
188+
}
189+
EXPECT_EQ(metadata_files, 0);
190+
}
191+
192+
TEST_F(InMemoryCatalogTest, RegisterTableNonexistentNamespace) {
193+
TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name = "t1"};
194+
195+
ICEBERG_UNWRAP_OR_FAIL(auto metadata,
196+
ReadTableMetadataFromResource("TableMetadataV2Valid.json"));
197+
auto table_location = GenerateTestTableLocation(table_ident.name);
198+
auto metadata_location = std::format("{}v1.metadata.json", table_location);
199+
ASSERT_THAT(TableMetadataUtil::Write(*file_io_, metadata_location, *metadata), IsOk());
200+
201+
// Registering into a nonexistent namespace reports NoSuchNamespace. The dead
202+
// if (!NamespaceExists(...)) check tested the Result<bool>'s has_value() rather
203+
// than the contained bool, so this previously fell through to kUnknownError.
204+
auto table = catalog_->RegisterTable(table_ident, metadata_location);
205+
EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace));
206+
}
207+
161208
TEST_F(InMemoryCatalogTest, RefreshTable) {
162209
TableIdentifier table_ident{.ns = {}, .name = "t1"};
163210
auto schema = std::make_shared<Schema>(

0 commit comments

Comments
 (0)