This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new bbc23591 fix(catalog): validate namespace existence in InMemoryCatalog
create and register (#980)
bbc23591 is described below
commit bbc235910706c44dcee69202d3d8c165cede6992
Author: YangJie <[email protected]>
AuthorDate: Tue Oct 13 03:15:27 2026 -0400
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 atom [...]
## 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`.
---
src/iceberg/catalog/memory/in_memory_catalog.cc | 9 ++++-
src/iceberg/test/in_memory_catalog_test.cc | 47 +++++++++++++++++++++++++
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/iceberg/catalog/memory/in_memory_catalog.cc
b/src/iceberg/catalog/memory/in_memory_catalog.cc
index e4cf440d..2ca19833 100644
--- a/src/iceberg/catalog/memory/in_memory_catalog.cc
+++ b/src/iceberg/catalog/memory/in_memory_catalog.cc
@@ -422,6 +422,11 @@ Result<std::shared_ptr<Table>>
InMemoryCatalog::CreateTable(
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) {
std::unique_lock lock(mutex_);
+ ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists,
+ root_namespace_->NamespaceExists(identifier.ns));
+ if (!namespace_exists) {
+ return NoSuchNamespace("Table namespace does not exist: {}",
identifier.ns);
+ }
if (root_namespace_->TableExists(identifier).value_or(false)) {
return AlreadyExists("Table already exists: {}", identifier);
}
@@ -609,7 +614,9 @@ Result<std::shared_ptr<Table>>
InMemoryCatalog::RegisterTable(
TableMetadataUtil::Read(*file_io_,
metadata_file_location));
std::unique_lock lock(mutex_);
- if (!root_namespace_->NamespaceExists(identifier.ns)) {
+ ICEBERG_ASSIGN_OR_RAISE(auto namespace_exists,
+ root_namespace_->NamespaceExists(identifier.ns));
+ if (!namespace_exists) {
return NoSuchNamespace("Table namespace does not exist: {}",
identifier.ns);
}
if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) {
diff --git a/src/iceberg/test/in_memory_catalog_test.cc
b/src/iceberg/test/in_memory_catalog_test.cc
index b2c88f57..db90aa14 100644
--- a/src/iceberg/test/in_memory_catalog_test.cc
+++ b/src/iceberg/test/in_memory_catalog_test.cc
@@ -158,6 +158,53 @@ TEST_F(InMemoryCatalogTest, RegisterTable) {
ASSERT_EQ(table.value()->location(), "s3://bucket/test/location");
}
+TEST_F(InMemoryCatalogTest, CreateTableNonexistentNamespace) {
+ TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name =
"t1"};
+ auto schema = std::make_shared<Schema>(
+ std::vector<SchemaField>{SchemaField::MakeRequired(1, "id", int64())},
+ /*schema_id=*/1);
+ auto spec = PartitionSpec::Unpartitioned();
+ auto sort_order = SortOrder::Unsorted();
+
+ // Use an explicit location whose metadata directory already exists (the
local
+ // FileIO does not create parent dirs). A write-before-validate bug would
land a
+ // detectable orphan there. GenerateTestTableLocation is unique per test and
+ // auto-cleaned via created_temp_paths_.
+ auto table_location = GenerateTestTableLocation(table_ident.name);
+
+ auto table =
+ catalog_->CreateTable(table_ident, schema, spec, sort_order,
table_location, {});
+ EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace));
+
+ // The namespace check must run before any metadata file is written, so a
+ // failed create must leave no orphaned metadata file behind.
+ std::error_code ec;
+ size_t metadata_files = 0;
+ for (auto it = std::filesystem::recursive_directory_iterator(table_location,
ec);
+ it != std::filesystem::recursive_directory_iterator();
it.increment(ec)) {
+ if (it->path().extension() == ".json") {
+ ++metadata_files;
+ }
+ }
+ EXPECT_EQ(metadata_files, 0);
+}
+
+TEST_F(InMemoryCatalogTest, RegisterTableNonexistentNamespace) {
+ TableIdentifier table_ident{.ns = Namespace{.levels = {"missing"}}, .name =
"t1"};
+
+ 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());
+
+ // Registering into a nonexistent namespace reports NoSuchNamespace. The dead
+ // if (!NamespaceExists(...)) check tested the Result<bool>'s has_value()
rather
+ // than the contained bool, so this previously fell through to kUnknownError.
+ auto table = catalog_->RegisterTable(table_ident, metadata_location);
+ EXPECT_THAT(table, IsError(ErrorKind::kNoSuchNamespace));
+}
+
TEST_F(InMemoryCatalogTest, RefreshTable) {
TableIdentifier table_ident{.ns = {}, .name = "t1"};
auto schema = std::make_shared<Schema>(
