From 87d747f35e22b83908729aefdbcb6caa2044602f Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Thu, 17 Apr 2025 14:59:22 +0800 Subject: [PATCH 1/9] feat: implement initial MemoryCatalog functionality with namespace and table support --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/catalog/memory_catalog.cc | 248 ++++++++++++++++++++++++++ src/iceberg/catalog/memory_catalog.h | 228 +++++++++++++++++++++++ src/iceberg/type_fwd.h | 1 + test/CMakeLists.txt | 5 + test/memory_catalog_test.cc | 146 +++++++++++++++ 6 files changed, 629 insertions(+) create mode 100644 src/iceberg/catalog/memory_catalog.cc create mode 100644 src/iceberg/catalog/memory_catalog.h create mode 100644 test/memory_catalog_test.cc diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 44b5003e7..4072972ae 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -19,6 +19,7 @@ set(ICEBERG_INCLUDES "$" "$") set(ICEBERG_SOURCES arrow_c_data_internal.cc + catalog/memory_catalog.cc demo.cc expression/expression.cc file_reader.cc diff --git a/src/iceberg/catalog/memory_catalog.cc b/src/iceberg/catalog/memory_catalog.cc new file mode 100644 index 000000000..8d33f36a7 --- /dev/null +++ b/src/iceberg/catalog/memory_catalog.cc @@ -0,0 +1,248 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/memory_catalog.h" + +#include +#include // IWYU pragma: keep + +#include "iceberg/exception.h" +#include "iceberg/table.h" + +namespace iceberg { + +MemoryCatalog::MemoryCatalog(std::shared_ptr file_io, + std::optional warehouse_location) + : file_io_(std::move(file_io)), + warehouse_location_(std::move(warehouse_location)), + root_container_(std::make_unique()) {} + +void MemoryCatalog::Initialize( + const std::string& name, + const std::unordered_map& properties) { + catalog_name_ = name; + properties_ = properties; +} + +std::string_view MemoryCatalog::name() const { return catalog_name_; } + +Result> MemoryCatalog::ListTables( + const Namespace& ns) const { + std::unique_lock lock(mutex_); + const auto& table_names = root_container_->ListTables(ns); + std::vector table_idents; + table_idents.reserve(table_names.size()); + std::ranges::transform( + table_names, std::back_inserter(table_idents), + [&ns](auto const& table_name) { return TableIdentifier(ns, table_name); }); + return table_idents; +} + +Result> MemoryCatalog::CreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) { + throw IcebergError("not implemented"); +} + +Result> MemoryCatalog::UpdateTable( + const TableIdentifier& identifier, + const std::vector>& requirements, + const std::vector>& updates) { + throw IcebergError("not implemented"); +} + +Result> MemoryCatalog::StageCreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) { + throw IcebergError("not implemented"); +} + +bool MemoryCatalog::TableExists(const TableIdentifier& identifier) const { + std::unique_lock lock(mutex_); + return root_container_->TableExists(identifier); +} + +bool MemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { + std::unique_lock lock(mutex_); + // TODO(Guotao): Delete all metadata files if purge is true. + return root_container_->UnregisterTable(identifier); +} + +Result> MemoryCatalog::LoadTable( + const TableIdentifier& identifier) const { + throw IcebergError("not implemented"); +} + +Result> MemoryCatalog::RegisterTable( + const TableIdentifier& identifier, const std::string& metadata_file_location) { + std::unique_lock lock(mutex_); + if (!root_container_->NamespaceExists(identifier.ns)) { + return unexpected({.kind = ErrorKind::kNoSuchNamespace, + .message = "table namespace does not exist"}); + } + if (!root_container_->RegisterTable(identifier, metadata_file_location)) { + return unexpected( + {.kind = ErrorKind::kUnknownError, .message = "The registry failed."}); + } + return LoadTable(identifier); +} + +std::unique_ptr MemoryCatalog::BuildTable(const TableIdentifier& identifier, + const Schema& schema) const { + throw IcebergError("not implemented"); +} + +/// Implementation of NamespaceContainer +NamespaceContainer* NamespaceContainer::GetNamespaceContainer( + NamespaceContainer* root, const Namespace& namespace_ident) { + return GetNamespaceContainerImpl(root, namespace_ident); +} + +const NamespaceContainer* NamespaceContainer::GetNamespaceContainer( + const NamespaceContainer* root, const Namespace& namespace_ident) { + return GetNamespaceContainerImpl(root, namespace_ident); +} + +bool NamespaceContainer::NamespaceExists(const Namespace& namespace_ident) const { + return GetNamespaceContainer(this, namespace_ident) != nullptr; +} + +std::vector NamespaceContainer::ListChildrenNamespaces( + const std::optional& parent_namespace_ident) const { + auto container = this; + if (parent_namespace_ident.has_value()) { + container = GetNamespaceContainer(this, *parent_namespace_ident); + if (!container) return {}; + } + + std::vector names; + auto const& children = container->children_; + names.reserve(children.size()); + std::ranges::transform(children, std::back_inserter(names), + [](const auto& pair) { return pair.first; }); + return names; +} + +bool NamespaceContainer::CreateNamespace( + const Namespace& namespace_ident, + const std::unordered_map& properties) { + auto container = this; + bool newly_created = false; + + for (const auto& part_level : namespace_ident.levels) { + if (auto it = container->children_.find(part_level); + it == container->children_.end()) { + container = &container->children_[part_level]; + newly_created = true; + } else { + container = &it->second; + } + } + + if (!newly_created) return false; + + container->properties_ = properties; + return true; +} + +bool NamespaceContainer::DeleteNamespace(const Namespace& namespace_ident) { + if (namespace_ident.levels.empty()) return false; + + auto parent_namespace_ident = namespace_ident; + const auto to_delete = parent_namespace_ident.levels.back(); + parent_namespace_ident.levels.pop_back(); + + auto* parent = GetNamespaceContainer(this, parent_namespace_ident); + if (!parent) return false; + + auto it = parent->children_.find(to_delete); + if (it == parent->children_.end()) return false; + + const auto& target = it->second; + if (!target.children_.empty() || !target.table_metadata_locations_.empty()) { + return false; + } + + return parent->children_.erase(to_delete) > 0; +} + +std::optional> +NamespaceContainer::GetProperties(const Namespace& namespace_ident) const { + const auto container = GetNamespaceContainer(this, namespace_ident); + if (!container) return std::nullopt; + return container->properties_; +} + +bool NamespaceContainer::ReplaceProperties( + const Namespace& namespace_ident, + const std::unordered_map& properties) { + const auto container = GetNamespaceContainer(this, namespace_ident); + if (!container) return false; + container->properties_ = properties; + return true; +} + +std::vector NamespaceContainer::ListTables( + const Namespace& namespace_ident) const { + const auto container = GetNamespaceContainer(this, namespace_ident); + if (!container) return {}; + + const auto& locations = container->table_metadata_locations_; + std::vector table_names; + table_names.reserve(locations.size()); + + std::ranges::transform(locations, std::back_inserter(table_names), + [](const auto& pair) { return pair.first; }); + std::ranges::sort(table_names); + + return table_names; +} + +bool NamespaceContainer::RegisterTable(TableIdentifier const& table_ident, + const std::string& metadata_location) { + const auto container = GetNamespaceContainer(this, table_ident.ns); + if (!container) return false; + if (container->table_metadata_locations_.contains(table_ident.name)) return false; + container->table_metadata_locations_[table_ident.name] = metadata_location; + return true; +} + +bool NamespaceContainer::UnregisterTable(TableIdentifier const& table_ident) { + const auto container = GetNamespaceContainer(this, table_ident.ns); + if (!container) return false; + return container->table_metadata_locations_.erase(table_ident.name) > 0; +} + +bool NamespaceContainer::TableExists(TableIdentifier const& table_ident) const { + const auto container = GetNamespaceContainer(this, table_ident.ns); + if (!container) return false; + return container->table_metadata_locations_.contains(table_ident.name); +} + +std::optional NamespaceContainer::GetTableMetadataLocation( + TableIdentifier const& table_ident) const { + const auto container = GetNamespaceContainer(this, table_ident.ns); + if (!container) return std::nullopt; + const auto it = container->table_metadata_locations_.find(table_ident.name); + if (it == container->table_metadata_locations_.end()) return std::nullopt; + return it->second; +} +} // namespace iceberg diff --git a/src/iceberg/catalog/memory_catalog.h b/src/iceberg/catalog/memory_catalog.h new file mode 100644 index 000000000..236e3a4c5 --- /dev/null +++ b/src/iceberg/catalog/memory_catalog.h @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include +#include +#include + +#include "iceberg/catalog.h" + +namespace iceberg { + +class NamespaceContainer; + +class ICEBERG_EXPORT MemoryCatalog : public Catalog { + public: + MemoryCatalog(std::shared_ptr file_io, + std::optional warehouse_location); + + void Initialize( + const std::string& name, + const std::unordered_map& properties) override; + + std::string_view name() const override; + + Result> ListTables(const Namespace& ns) const override; + + Result> CreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) override; + + Result> UpdateTable( + const TableIdentifier& identifier, + const std::vector>& requirements, + const std::vector>& updates) override; + + Result> StageCreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) override; + + bool TableExists(const TableIdentifier& identifier) const override; + + bool DropTable(const TableIdentifier& identifier, bool purge) override; + + Result> LoadTable( + const TableIdentifier& identifier) const override; + + Result> RegisterTable( + const TableIdentifier& identifier, + const std::string& metadata_file_location) override; + + std::unique_ptr BuildTable(const TableIdentifier& identifier, + const Schema& schema) const override; + + private: + std::string catalog_name_; + std::unordered_map properties_; + std::shared_ptr file_io_; + std::optional warehouse_location_; + std::unique_ptr root_container_; + mutable std::recursive_mutex mutex_; +}; + +/** + * @brief A hierarchical container that manages namespaces and table metadata in-memory. + * + * Each NamespaceContainer represents a namespace level and can contain properties, + * tables, and child namespaces. This structure enables a tree-like representation + * of nested namespaces. + */ +class ICEBERG_EXPORT NamespaceContainer { + public: + /** + * @brief Checks whether the given namespace exists. + * @param namespace_ident The namespace to check. + * @return True if the namespace exists; false otherwise. + */ + bool NamespaceExists(const Namespace& namespace_ident) const; + + /** + * @brief Lists immediate child namespaces under the given parent namespace. + * @param parent_namespace_ident The optional parent namespace. If not provided, + * the children of the root are returned. + * @return A vector of child namespace names. + */ + std::vector ListChildrenNamespaces( + const std::optional& parent_namespace_ident = std::nullopt) const; + + /** + * @brief Creates a new namespace with the specified properties. + * @param namespace_ident The namespace to create. + * @param properties A map of key-value pairs to associate with the namespace. + * @return True if the namespace was successfully created; false if it already exists. + */ + bool CreateNamespace(const Namespace& namespace_ident, + const std::unordered_map& properties); + + /** + * @brief Deletes an existing namespace. + * @param namespace_ident The namespace to delete. + * @return True if the namespace was successfully deleted; false if it does not exist. + */ + bool DeleteNamespace(const Namespace& namespace_ident); + + /** + * @brief Retrieves the properties of the specified namespace. + * @param namespace_ident The namespace whose properties to retrieve. + * @return An optional containing the properties map if the namespace exists; + * std::nullopt otherwise. + */ + std::optional> GetProperties( + const Namespace& namespace_ident) const; + + /** + * @brief Replaces all properties of the given namespace. + * @param namespace_ident The namespace whose properties will be replaced. + * @param properties The new properties map. + * @return True if the namespace exists and properties were replaced; false otherwise. + */ + bool ReplaceProperties(const Namespace& namespace_ident, + const std::unordered_map& properties); + + /** + * @brief Lists all table names under the specified namespace. + * @param namespace_ident The namespace from which to list tables. + * @return A vector of table names. + */ + std::vector ListTables(const Namespace& namespace_ident) const; + + /** + * @brief Registers a table in the given namespace with a metadata location. + * @param table_ident The fully qualified identifier of the table. + * @param metadata_location The path to the table's metadata. + * @return True if the table was registered successfully; false otherwise. + */ + bool RegisterTable(TableIdentifier const& table_ident, + const std::string& metadata_location); + + /** + * @brief Unregisters a table from the specified namespace. + * @param table_ident The identifier of the table to unregister. + * @return True if the table existed and was removed; false otherwise. + */ + bool UnregisterTable(TableIdentifier const& table_ident); + + /** + * @brief Checks if a table exists in the specified namespace. + * @param table_ident The identifier of the table to check. + * @return True if the table exists; false otherwise. + */ + bool TableExists(TableIdentifier const& table_ident) const; + + /** + * @brief Gets the metadata location for the specified table. + * @param table_ident The identifier of the table. + * @return An optional string containing the metadata location if the table exists; + * std::nullopt otherwise. + */ + std::optional GetTableMetadataLocation( + TableIdentifier const& table_ident) const; + + private: + /** + * @brief Helper function to retrieve the container node for a given namespace. + * @param root The root of the tree to search. + * @param namespace_ident The namespace path to traverse. + * @return A pointer to the corresponding NamespaceContainer if it exists; nullptr + * otherwise. + */ + static NamespaceContainer* GetNamespaceContainer(NamespaceContainer* root, + const Namespace& namespace_ident); + + /** + * @brief Const version of GetNamespaceContainer. + */ + static const NamespaceContainer* GetNamespaceContainer( + const NamespaceContainer* root, const Namespace& namespace_ident); + + /** + * @brief Templated implementation for retrieving a NamespaceContainer node. + * @tparam ContainerPtr Pointer type to NamespaceContainer (const or non-const). + * @param root The root node. + * @param namespace_ident The namespace path. + * @return A pointer to the container node if found; nullptr otherwise. + */ + template + static ContainerPtr GetNamespaceContainerImpl(ContainerPtr root, + const Namespace& namespace_ident) { + auto node = root; + for (const auto& part_level : namespace_ident.levels) { + auto it = node->children_.find(part_level); + if (it == node->children_.end()) { + return nullptr; + } + node = &it->second; + } + return node; + } + + /// Map of child namespace names to their corresponding container instances. + std::unordered_map children_; + + /// Key-value property map for this namespace. + std::unordered_map properties_; + + /// Mapping of table names to metadata file locations. + std::unordered_map table_metadata_locations_; +}; +} // namespace iceberg diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 9fc6bd6cb..a5996c426 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -87,6 +87,7 @@ struct Namespace; struct TableIdentifier; class Catalog; +class FileIO; class LocationProvider; class SortField; class SortOrder; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 4a88229f5..00feb8556 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -43,6 +43,11 @@ target_sources(schema_test target_link_libraries(schema_test PRIVATE iceberg_static GTest::gtest_main GTest::gmock) add_test(NAME schema_test COMMAND schema_test) +add_executable(catalog_test) +target_sources(catalog_test PRIVATE memory_catalog_test.cc) +target_link_libraries(catalog_test PRIVATE iceberg_static GTest::gtest_main GTest::gmock) +add_test(NAME catalog_test COMMAND catalog_test) + add_executable(expression_test) target_sources(expression_test PRIVATE expression_test.cc) target_link_libraries(expression_test PRIVATE iceberg_static GTest::gtest_main diff --git a/test/memory_catalog_test.cc b/test/memory_catalog_test.cc new file mode 100644 index 000000000..f2e5d0625 --- /dev/null +++ b/test/memory_catalog_test.cc @@ -0,0 +1,146 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/memory_catalog.h" + +#include +#include + +namespace iceberg { + +class NamespaceContainerTest : public ::testing::Test { + protected: + NamespaceContainer container; + + static Namespace MakeNs(std::vector levels) { + return Namespace{std::move(levels)}; + } + + static TableIdentifier MakeTable(const std::vector& ns, + const std::string& name) { + return TableIdentifier{.ns = Namespace{ns}, .name = name}; + } +}; + +TEST_F(NamespaceContainerTest, CreateAndExistsNamespace) { + Namespace ns = MakeNs({"a", "b"}); + EXPECT_FALSE(container.NamespaceExists(ns)); + EXPECT_TRUE(container.CreateNamespace(ns, {{"k", "v"}})); + EXPECT_TRUE(container.NamespaceExists(ns)); +} + +TEST_F(NamespaceContainerTest, CreateNamespaceTwiceFails) { + Namespace ns = MakeNs({"x", "y"}); + EXPECT_TRUE(container.CreateNamespace(ns, {{"a", "b"}})); + EXPECT_FALSE(container.CreateNamespace(ns, {{"a", "b"}})); +} + +TEST_F(NamespaceContainerTest, DeleteEmptyNamespaceSucceeds) { + Namespace ns = MakeNs({"del", "me"}); + EXPECT_TRUE(container.CreateNamespace(ns, {})); + EXPECT_TRUE(container.DeleteNamespace(ns)); + EXPECT_FALSE(container.NamespaceExists(ns)); +} + +TEST_F(NamespaceContainerTest, DeleteNonEmptyNamespaceFails) { + Namespace ns = MakeNs({"del", "fail"}); + EXPECT_TRUE(container.CreateNamespace(ns, {})); + auto table = MakeTable({"del", "fail"}, "t1"); + EXPECT_TRUE(container.RegisterTable(table, "loc1")); + EXPECT_FALSE(container.DeleteNamespace(ns)); +} + +TEST_F(NamespaceContainerTest, ReplaceAndGetProperties) { + Namespace ns = MakeNs({"props"}); + EXPECT_TRUE(container.CreateNamespace(ns, {{"x", "1"}})); + EXPECT_TRUE(container.ReplaceProperties(ns, {{"x", "2"}, {"y", "3"}})); + + auto props = container.GetProperties(ns); + ASSERT_TRUE(props.has_value()); + EXPECT_EQ(props->at("x"), "2"); + EXPECT_EQ(props->at("y"), "3"); +} + +TEST_F(NamespaceContainerTest, GetPropertiesNonExistReturnsNullopt) { + Namespace ns = MakeNs({"unknown"}); + EXPECT_FALSE(container.GetProperties(ns).has_value()); +} + +TEST_F(NamespaceContainerTest, RegisterAndLookupTable) { + Namespace ns = MakeNs({"tbl", "ns"}); + EXPECT_TRUE(container.CreateNamespace(ns, {})); + auto t1 = MakeTable(ns.levels, "table1"); + EXPECT_TRUE(container.RegisterTable(t1, "metadata/path/1")); + + EXPECT_TRUE(container.TableExists(t1)); + auto location = container.GetTableMetadataLocation(t1); + ASSERT_TRUE(location.has_value()); + EXPECT_EQ(location.value(), "metadata/path/1"); +} + +TEST_F(NamespaceContainerTest, UnregisterTable) { + Namespace ns = MakeNs({"tbl", "unreg"}); + EXPECT_TRUE(container.CreateNamespace(ns, {})); + auto t = MakeTable(ns.levels, "t2"); + EXPECT_TRUE(container.RegisterTable(t, "meta2")); + EXPECT_TRUE(container.UnregisterTable(t)); + EXPECT_FALSE(container.TableExists(t)); +} + +TEST_F(NamespaceContainerTest, RegisterTableOnNonExistingNamespaceFails) { + auto t = MakeTable({"nonexist", "ns"}, "oops"); + EXPECT_FALSE(container.RegisterTable(t, "x")); +} + +TEST_F(NamespaceContainerTest, ListChildrenNamespaces) { + EXPECT_TRUE(container.CreateNamespace(MakeNs({"a"}), {})); + EXPECT_TRUE(container.CreateNamespace(MakeNs({"a", "b"}), {})); + EXPECT_TRUE(container.CreateNamespace(MakeNs({"a", "c"}), {})); + auto children = container.ListChildrenNamespaces(MakeNs({"a"})); + EXPECT_THAT(children, ::testing::UnorderedElementsAre("b", "c")); +} + +TEST_F(NamespaceContainerTest, ListTablesReturnsCorrectNames) { + Namespace ns = MakeNs({"list", "tables"}); + EXPECT_TRUE(container.CreateNamespace(ns, {})); + EXPECT_TRUE(container.RegisterTable(MakeTable(ns.levels, "a"), "loc_a")); + EXPECT_TRUE(container.RegisterTable(MakeTable(ns.levels, "b"), "loc_b")); + EXPECT_TRUE(container.RegisterTable(MakeTable(ns.levels, "c"), "loc_c")); + + auto tables = container.ListTables(ns); + EXPECT_THAT(tables, ::testing::UnorderedElementsAre("a", "b", "c")); +} + +class MemoryCatalogTest : public ::testing::Test { + protected: + void SetUp() override { + file_io_ = nullptr; // TODO(Guotao): A real FileIO instance needs to be constructed. + catalog_ = std::make_unique(file_io_, std::nullopt); + catalog_->Initialize("test_catalog", {{"prop1", "val1"}}); + } + + std::shared_ptr file_io_; + std::unique_ptr catalog_; +}; + +TEST_F(MemoryCatalogTest, NameAndInitialization) { + EXPECT_EQ(catalog_->name(), "test_catalog"); +} + +} // namespace iceberg From 33c02c3ce8e201f4a72165d634cdd8f3125de61f Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Sat, 19 Apr 2025 15:24:05 +0800 Subject: [PATCH 2/9] fix: some review suggestions --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/catalog/CMakeLists.txt | 18 +++++ src/iceberg/catalog/memory_catalog.cc | 42 ++++++----- src/iceberg/catalog/memory_catalog.h | 104 ++++++++++---------------- test/memory_catalog_test.cc | 2 +- 5 files changed, 85 insertions(+), 82 deletions(-) create mode 100644 src/iceberg/catalog/CMakeLists.txt diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 4072972ae..121a2ffc0 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -75,6 +75,7 @@ add_iceberg_lib(iceberg iceberg_install_all_headers(iceberg) +add_subdirectory(catalog) add_subdirectory(expression) add_subdirectory(util) diff --git a/src/iceberg/catalog/CMakeLists.txt b/src/iceberg/catalog/CMakeLists.txt new file mode 100644 index 000000000..ec53e84aa --- /dev/null +++ b/src/iceberg/catalog/CMakeLists.txt @@ -0,0 +1,18 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +iceberg_install_all_headers(iceberg/catalog) diff --git a/src/iceberg/catalog/memory_catalog.cc b/src/iceberg/catalog/memory_catalog.cc index 8d33f36a7..3b9acb777 100644 --- a/src/iceberg/catalog/memory_catalog.cc +++ b/src/iceberg/catalog/memory_catalog.cc @@ -27,8 +27,22 @@ namespace iceberg { +namespace { + +NamespaceContainer* GetNamespaceContainer(NamespaceContainer* root, + const Namespace& namespace_ident) { + return NamespaceContainer::GetNamespaceContainerImpl(root, namespace_ident); +} + +const NamespaceContainer* GetNamespaceContainer(const NamespaceContainer* root, + const Namespace& namespace_ident) { + return NamespaceContainer::GetNamespaceContainerImpl(root, namespace_ident); +} + +} // namespace + MemoryCatalog::MemoryCatalog(std::shared_ptr file_io, - std::optional warehouse_location) + std::string warehouse_location) : file_io_(std::move(file_io)), warehouse_location_(std::move(warehouse_location)), root_container_(std::make_unique()) {} @@ -58,21 +72,24 @@ Result> MemoryCatalog::CreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, const std::unordered_map& properties) { - throw IcebergError("not implemented"); + return unexpected( + {.kind = ErrorKind::kNotImplemented, .message = "CreateTable"}); } Result> MemoryCatalog::UpdateTable( const TableIdentifier& identifier, const std::vector>& requirements, const std::vector>& updates) { - throw IcebergError("not implemented"); + return unexpected( + {.kind = ErrorKind::kNotImplemented, .message = "UpdateTable"}); } Result> MemoryCatalog::StageCreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, const std::unordered_map& properties) { - throw IcebergError("not implemented"); + return unexpected( + {.kind = ErrorKind::kNotImplemented, .message = "StageCreateTable"}); } bool MemoryCatalog::TableExists(const TableIdentifier& identifier) const { @@ -88,7 +105,7 @@ bool MemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { Result> MemoryCatalog::LoadTable( const TableIdentifier& identifier) const { - throw IcebergError("not implemented"); + return unexpected({.kind = ErrorKind::kNotImplemented, .message = "LoadTable"}); } Result> MemoryCatalog::RegisterTable( @@ -110,17 +127,6 @@ std::unique_ptr MemoryCatalog::BuildTable(const TableIdentifier& i throw IcebergError("not implemented"); } -/// Implementation of NamespaceContainer -NamespaceContainer* NamespaceContainer::GetNamespaceContainer( - NamespaceContainer* root, const Namespace& namespace_ident) { - return GetNamespaceContainerImpl(root, namespace_ident); -} - -const NamespaceContainer* NamespaceContainer::GetNamespaceContainer( - const NamespaceContainer* root, const Namespace& namespace_ident) { - return GetNamespaceContainerImpl(root, namespace_ident); -} - bool NamespaceContainer::NamespaceExists(const Namespace& namespace_ident) const { return GetNamespaceContainer(this, namespace_ident) != nullptr; } @@ -157,7 +163,9 @@ bool NamespaceContainer::CreateNamespace( } } - if (!newly_created) return false; + if (!newly_created) { + return false; + } container->properties_ = properties; return true; diff --git a/src/iceberg/catalog/memory_catalog.h b/src/iceberg/catalog/memory_catalog.h index 236e3a4c5..8dbd9b4ae 100644 --- a/src/iceberg/catalog/memory_catalog.h +++ b/src/iceberg/catalog/memory_catalog.h @@ -31,8 +31,7 @@ class NamespaceContainer; class ICEBERG_EXPORT MemoryCatalog : public Catalog { public: - MemoryCatalog(std::shared_ptr file_io, - std::optional warehouse_location); + MemoryCatalog(std::shared_ptr file_io, std::string warehouse_location); void Initialize( const std::string& name, @@ -75,13 +74,13 @@ class ICEBERG_EXPORT MemoryCatalog : public Catalog { std::string catalog_name_; std::unordered_map properties_; std::shared_ptr file_io_; - std::optional warehouse_location_; + std::string warehouse_location_; std::unique_ptr root_container_; mutable std::recursive_mutex mutex_; }; /** - * @brief A hierarchical container that manages namespaces and table metadata in-memory. + * \brief A hierarchical container that manages namespaces and table metadata in-memory. * * Each NamespaceContainer represents a namespace level and can contain properties, * tables, and child namespaces. This structure enables a tree-like representation @@ -90,118 +89,94 @@ class ICEBERG_EXPORT MemoryCatalog : public Catalog { class ICEBERG_EXPORT NamespaceContainer { public: /** - * @brief Checks whether the given namespace exists. - * @param namespace_ident The namespace to check. - * @return True if the namespace exists; false otherwise. + * \brief Checks whether the given namespace exists. + * \param[in] namespace_ident The namespace to check. + * \return True if the namespace exists; false otherwise. */ bool NamespaceExists(const Namespace& namespace_ident) const; /** - * @brief Lists immediate child namespaces under the given parent namespace. - * @param parent_namespace_ident The optional parent namespace. If not provided, + * \brief Lists immediate child namespaces under the given parent namespace. + * \param[in] parent_namespace_ident The optional parent namespace. If not provided, * the children of the root are returned. - * @return A vector of child namespace names. + * \return A vector of child namespace names. */ std::vector ListChildrenNamespaces( const std::optional& parent_namespace_ident = std::nullopt) const; /** - * @brief Creates a new namespace with the specified properties. - * @param namespace_ident The namespace to create. - * @param properties A map of key-value pairs to associate with the namespace. - * @return True if the namespace was successfully created; false if it already exists. + * \brief Creates a new namespace with the specified properties. + * \param[in] namespace_ident The namespace to create. + * \param[in] properties A map of key-value pairs to associate with the namespace. + * \return True if the namespace was successfully created; false if it already exists. */ bool CreateNamespace(const Namespace& namespace_ident, const std::unordered_map& properties); /** - * @brief Deletes an existing namespace. - * @param namespace_ident The namespace to delete. - * @return True if the namespace was successfully deleted; false if it does not exist. + * \brief Deletes an existing namespace. + * \param[in] namespace_ident The namespace to delete. + * \return True if the namespace was successfully deleted; false if it does not exist. */ bool DeleteNamespace(const Namespace& namespace_ident); /** - * @brief Retrieves the properties of the specified namespace. - * @param namespace_ident The namespace whose properties to retrieve. - * @return An optional containing the properties map if the namespace exists; + * \brief Retrieves the properties of the specified namespace. + * \param[in] namespace_ident The namespace whose properties to retrieve. + * \return An optional containing the properties map if the namespace exists; * std::nullopt otherwise. */ std::optional> GetProperties( const Namespace& namespace_ident) const; /** - * @brief Replaces all properties of the given namespace. - * @param namespace_ident The namespace whose properties will be replaced. - * @param properties The new properties map. - * @return True if the namespace exists and properties were replaced; false otherwise. + * \brief Replaces all properties of the given namespace. + * \param[in] namespace_ident The namespace whose properties will be replaced. + * \param[in] properties The new properties map. + * \return True if the namespace exists and properties were replaced; false otherwise. */ bool ReplaceProperties(const Namespace& namespace_ident, const std::unordered_map& properties); /** - * @brief Lists all table names under the specified namespace. - * @param namespace_ident The namespace from which to list tables. - * @return A vector of table names. + * \brief Lists all table names under the specified namespace. + * \param[in] namespace_ident The namespace from which to list tables. + * \return A vector of table names. */ std::vector ListTables(const Namespace& namespace_ident) const; /** - * @brief Registers a table in the given namespace with a metadata location. - * @param table_ident The fully qualified identifier of the table. - * @param metadata_location The path to the table's metadata. - * @return True if the table was registered successfully; false otherwise. + * \brief Registers a table in the given namespace with a metadata location. + * \param[in] table_ident The fully qualified identifier of the table. + * \param[in] metadata_location The path to the table's metadata. + * \return True if the table was registered successfully; false otherwise. */ bool RegisterTable(TableIdentifier const& table_ident, const std::string& metadata_location); /** - * @brief Unregisters a table from the specified namespace. - * @param table_ident The identifier of the table to unregister. - * @return True if the table existed and was removed; false otherwise. + * \brief Unregisters a table from the specified namespace. + * \param[in] table_ident The identifier of the table to unregister. + * \return True if the table existed and was removed; false otherwise. */ bool UnregisterTable(TableIdentifier const& table_ident); /** - * @brief Checks if a table exists in the specified namespace. - * @param table_ident The identifier of the table to check. - * @return True if the table exists; false otherwise. + * \brief Checks if a table exists in the specified namespace. + * \param[in] table_ident The identifier of the table to check. + * \return True if the table exists; false otherwise. */ bool TableExists(TableIdentifier const& table_ident) const; /** - * @brief Gets the metadata location for the specified table. - * @param table_ident The identifier of the table. - * @return An optional string containing the metadata location if the table exists; + * \brief Gets the metadata location for the specified table. + * \param[in] table_ident The identifier of the table. + * \return An optional string containing the metadata location if the table exists; * std::nullopt otherwise. */ std::optional GetTableMetadataLocation( TableIdentifier const& table_ident) const; - private: - /** - * @brief Helper function to retrieve the container node for a given namespace. - * @param root The root of the tree to search. - * @param namespace_ident The namespace path to traverse. - * @return A pointer to the corresponding NamespaceContainer if it exists; nullptr - * otherwise. - */ - static NamespaceContainer* GetNamespaceContainer(NamespaceContainer* root, - const Namespace& namespace_ident); - - /** - * @brief Const version of GetNamespaceContainer. - */ - static const NamespaceContainer* GetNamespaceContainer( - const NamespaceContainer* root, const Namespace& namespace_ident); - - /** - * @brief Templated implementation for retrieving a NamespaceContainer node. - * @tparam ContainerPtr Pointer type to NamespaceContainer (const or non-const). - * @param root The root node. - * @param namespace_ident The namespace path. - * @return A pointer to the container node if found; nullptr otherwise. - */ template static ContainerPtr GetNamespaceContainerImpl(ContainerPtr root, const Namespace& namespace_ident) { @@ -216,6 +191,7 @@ class ICEBERG_EXPORT NamespaceContainer { return node; } + private: /// Map of child namespace names to their corresponding container instances. std::unordered_map children_; diff --git a/test/memory_catalog_test.cc b/test/memory_catalog_test.cc index f2e5d0625..9b09a60bf 100644 --- a/test/memory_catalog_test.cc +++ b/test/memory_catalog_test.cc @@ -131,7 +131,7 @@ class MemoryCatalogTest : public ::testing::Test { protected: void SetUp() override { file_io_ = nullptr; // TODO(Guotao): A real FileIO instance needs to be constructed. - catalog_ = std::make_unique(file_io_, std::nullopt); + catalog_ = std::make_unique(file_io_, "/tmp/warehouse/"); catalog_->Initialize("test_catalog", {{"prop1", "val1"}}); } From 0a588235f56443c014ff27daa534b63758198b40 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Thu, 24 Apr 2025 09:05:23 +0800 Subject: [PATCH 3/9] fix: Merge the latest commits from the main branch --- src/iceberg/catalog/memory_catalog.cc | 145 +++++++++++++------------- src/iceberg/catalog/memory_catalog.h | 24 ++--- test/memory_catalog_test.cc | 122 +++++++++++----------- 3 files changed, 145 insertions(+), 146 deletions(-) diff --git a/src/iceberg/catalog/memory_catalog.cc b/src/iceberg/catalog/memory_catalog.cc index 3b9acb777..3c354976a 100644 --- a/src/iceberg/catalog/memory_catalog.cc +++ b/src/iceberg/catalog/memory_catalog.cc @@ -29,37 +29,37 @@ namespace iceberg { namespace { -NamespaceContainer* GetNamespaceContainer(NamespaceContainer* root, - const Namespace& namespace_ident) { - return NamespaceContainer::GetNamespaceContainerImpl(root, namespace_ident); +InMemoryNamespace* GetNamespace(InMemoryNamespace* root, + const Namespace& namespace_ident) { + return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); } -const NamespaceContainer* GetNamespaceContainer(const NamespaceContainer* root, - const Namespace& namespace_ident) { - return NamespaceContainer::GetNamespaceContainerImpl(root, namespace_ident); +const InMemoryNamespace* GetNamespace(const InMemoryNamespace* root, + const Namespace& namespace_ident) { + return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); } } // namespace -MemoryCatalog::MemoryCatalog(std::shared_ptr file_io, - std::string warehouse_location) +InMemoryCatalog::InMemoryCatalog(std::shared_ptr file_io, + std::string warehouse_location) : file_io_(std::move(file_io)), warehouse_location_(std::move(warehouse_location)), - root_container_(std::make_unique()) {} + root_namespace_(std::make_unique()) {} -void MemoryCatalog::Initialize( +void InMemoryCatalog::Initialize( const std::string& name, const std::unordered_map& properties) { catalog_name_ = name; properties_ = properties; } -std::string_view MemoryCatalog::name() const { return catalog_name_; } +std::string_view InMemoryCatalog::name() const { return catalog_name_; } -Result> MemoryCatalog::ListTables( +Result> InMemoryCatalog::ListTables( const Namespace& ns) const { std::unique_lock lock(mutex_); - const auto& table_names = root_container_->ListTables(ns); + const auto& table_names = root_namespace_->ListTables(ns); std::vector table_idents; table_idents.reserve(table_names.size()); std::ranges::transform( @@ -68,7 +68,7 @@ Result> MemoryCatalog::ListTables( return table_idents; } -Result> MemoryCatalog::CreateTable( +Result> InMemoryCatalog::CreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, const std::unordered_map& properties) { @@ -76,7 +76,7 @@ Result> MemoryCatalog::CreateTable( {.kind = ErrorKind::kNotImplemented, .message = "CreateTable"}); } -Result> MemoryCatalog::UpdateTable( +Result> InMemoryCatalog::UpdateTable( const TableIdentifier& identifier, const std::vector>& requirements, const std::vector>& updates) { @@ -84,7 +84,7 @@ Result> MemoryCatalog::UpdateTable( {.kind = ErrorKind::kNotImplemented, .message = "UpdateTable"}); } -Result> MemoryCatalog::StageCreateTable( +Result> InMemoryCatalog::StageCreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, const std::unordered_map& properties) { @@ -92,74 +92,73 @@ Result> MemoryCatalog::StageCreateTable( {.kind = ErrorKind::kNotImplemented, .message = "StageCreateTable"}); } -bool MemoryCatalog::TableExists(const TableIdentifier& identifier) const { +bool InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { std::unique_lock lock(mutex_); - return root_container_->TableExists(identifier); + return root_namespace_->TableExists(identifier); } -bool MemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { +bool InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { std::unique_lock lock(mutex_); // TODO(Guotao): Delete all metadata files if purge is true. - return root_container_->UnregisterTable(identifier); + return root_namespace_->UnregisterTable(identifier); } -Result> MemoryCatalog::LoadTable( +Result> InMemoryCatalog::LoadTable( const TableIdentifier& identifier) const { return unexpected({.kind = ErrorKind::kNotImplemented, .message = "LoadTable"}); } -Result> MemoryCatalog::RegisterTable( +Result> InMemoryCatalog::RegisterTable( const TableIdentifier& identifier, const std::string& metadata_file_location) { std::unique_lock lock(mutex_); - if (!root_container_->NamespaceExists(identifier.ns)) { + if (!root_namespace_->NamespaceExists(identifier.ns)) { return unexpected({.kind = ErrorKind::kNoSuchNamespace, .message = "table namespace does not exist"}); } - if (!root_container_->RegisterTable(identifier, metadata_file_location)) { + if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) { return unexpected( {.kind = ErrorKind::kUnknownError, .message = "The registry failed."}); } return LoadTable(identifier); } -std::unique_ptr MemoryCatalog::BuildTable(const TableIdentifier& identifier, - const Schema& schema) const { +std::unique_ptr InMemoryCatalog::BuildTable( + const TableIdentifier& identifier, const Schema& schema) const { throw IcebergError("not implemented"); } -bool NamespaceContainer::NamespaceExists(const Namespace& namespace_ident) const { - return GetNamespaceContainer(this, namespace_ident) != nullptr; +bool InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const { + return GetNamespace(this, namespace_ident) != nullptr; } -std::vector NamespaceContainer::ListChildrenNamespaces( +std::vector InMemoryNamespace::ListChildrenNamespaces( const std::optional& parent_namespace_ident) const { - auto container = this; + auto ns = this; if (parent_namespace_ident.has_value()) { - container = GetNamespaceContainer(this, *parent_namespace_ident); - if (!container) return {}; + ns = GetNamespace(this, *parent_namespace_ident); + if (!ns) return {}; } std::vector names; - auto const& children = container->children_; + auto const& children = ns->children_; names.reserve(children.size()); std::ranges::transform(children, std::back_inserter(names), [](const auto& pair) { return pair.first; }); return names; } -bool NamespaceContainer::CreateNamespace( +bool InMemoryNamespace::CreateNamespace( const Namespace& namespace_ident, const std::unordered_map& properties) { - auto container = this; + auto ns = this; bool newly_created = false; for (const auto& part_level : namespace_ident.levels) { - if (auto it = container->children_.find(part_level); - it == container->children_.end()) { - container = &container->children_[part_level]; + if (auto it = ns->children_.find(part_level); it == ns->children_.end()) { + ns = &ns->children_[part_level]; newly_created = true; } else { - container = &it->second; + ns = &it->second; } } @@ -167,18 +166,18 @@ bool NamespaceContainer::CreateNamespace( return false; } - container->properties_ = properties; + ns->properties_ = properties; return true; } -bool NamespaceContainer::DeleteNamespace(const Namespace& namespace_ident) { +bool InMemoryNamespace::DeleteNamespace(const Namespace& namespace_ident) { if (namespace_ident.levels.empty()) return false; auto parent_namespace_ident = namespace_ident; const auto to_delete = parent_namespace_ident.levels.back(); parent_namespace_ident.levels.pop_back(); - auto* parent = GetNamespaceContainer(this, parent_namespace_ident); + auto* parent = GetNamespace(this, parent_namespace_ident); if (!parent) return false; auto it = parent->children_.find(to_delete); @@ -193,27 +192,27 @@ bool NamespaceContainer::DeleteNamespace(const Namespace& namespace_ident) { } std::optional> -NamespaceContainer::GetProperties(const Namespace& namespace_ident) const { - const auto container = GetNamespaceContainer(this, namespace_ident); - if (!container) return std::nullopt; - return container->properties_; +InMemoryNamespace::GetProperties(const Namespace& namespace_ident) const { + const auto ns = GetNamespace(this, namespace_ident); + if (!ns) return std::nullopt; + return ns->properties_; } -bool NamespaceContainer::ReplaceProperties( +bool InMemoryNamespace::ReplaceProperties( const Namespace& namespace_ident, const std::unordered_map& properties) { - const auto container = GetNamespaceContainer(this, namespace_ident); - if (!container) return false; - container->properties_ = properties; + const auto ns = GetNamespace(this, namespace_ident); + if (!ns) return false; + ns->properties_ = properties; return true; } -std::vector NamespaceContainer::ListTables( +std::vector InMemoryNamespace::ListTables( const Namespace& namespace_ident) const { - const auto container = GetNamespaceContainer(this, namespace_ident); - if (!container) return {}; + const auto ns = GetNamespace(this, namespace_ident); + if (!ns) return {}; - const auto& locations = container->table_metadata_locations_; + const auto& locations = ns->table_metadata_locations_; std::vector table_names; table_names.reserve(locations.size()); @@ -224,33 +223,33 @@ std::vector NamespaceContainer::ListTables( return table_names; } -bool NamespaceContainer::RegisterTable(TableIdentifier const& table_ident, - const std::string& metadata_location) { - const auto container = GetNamespaceContainer(this, table_ident.ns); - if (!container) return false; - if (container->table_metadata_locations_.contains(table_ident.name)) return false; - container->table_metadata_locations_[table_ident.name] = metadata_location; +bool InMemoryNamespace::RegisterTable(TableIdentifier const& table_ident, + const std::string& metadata_location) { + const auto ns = GetNamespace(this, table_ident.ns); + if (!ns) return false; + if (ns->table_metadata_locations_.contains(table_ident.name)) return false; + ns->table_metadata_locations_[table_ident.name] = metadata_location; return true; } -bool NamespaceContainer::UnregisterTable(TableIdentifier const& table_ident) { - const auto container = GetNamespaceContainer(this, table_ident.ns); - if (!container) return false; - return container->table_metadata_locations_.erase(table_ident.name) > 0; +bool InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) { + const auto ns = GetNamespace(this, table_ident.ns); + if (!ns) return false; + return ns->table_metadata_locations_.erase(table_ident.name) > 0; } -bool NamespaceContainer::TableExists(TableIdentifier const& table_ident) const { - const auto container = GetNamespaceContainer(this, table_ident.ns); - if (!container) return false; - return container->table_metadata_locations_.contains(table_ident.name); +bool InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const { + const auto ns = GetNamespace(this, table_ident.ns); + if (!ns) return false; + return ns->table_metadata_locations_.contains(table_ident.name); } -std::optional NamespaceContainer::GetTableMetadataLocation( +std::optional InMemoryNamespace::GetTableMetadataLocation( TableIdentifier const& table_ident) const { - const auto container = GetNamespaceContainer(this, table_ident.ns); - if (!container) return std::nullopt; - const auto it = container->table_metadata_locations_.find(table_ident.name); - if (it == container->table_metadata_locations_.end()) return std::nullopt; + const auto ns = GetNamespace(this, table_ident.ns); + if (!ns) return std::nullopt; + const auto it = ns->table_metadata_locations_.find(table_ident.name); + if (it == ns->table_metadata_locations_.end()) return std::nullopt; return it->second; } } // namespace iceberg diff --git a/src/iceberg/catalog/memory_catalog.h b/src/iceberg/catalog/memory_catalog.h index 8dbd9b4ae..eed075f1f 100644 --- a/src/iceberg/catalog/memory_catalog.h +++ b/src/iceberg/catalog/memory_catalog.h @@ -27,11 +27,11 @@ namespace iceberg { -class NamespaceContainer; +class InMemoryNamespace; -class ICEBERG_EXPORT MemoryCatalog : public Catalog { +class ICEBERG_EXPORT InMemoryCatalog : public Catalog { public: - MemoryCatalog(std::shared_ptr file_io, std::string warehouse_location); + InMemoryCatalog(std::shared_ptr file_io, std::string warehouse_location); void Initialize( const std::string& name, @@ -75,18 +75,18 @@ class ICEBERG_EXPORT MemoryCatalog : public Catalog { std::unordered_map properties_; std::shared_ptr file_io_; std::string warehouse_location_; - std::unique_ptr root_container_; + std::unique_ptr root_namespace_; mutable std::recursive_mutex mutex_; }; /** - * \brief A hierarchical container that manages namespaces and table metadata in-memory. + * \brief A hierarchical namespace that manages namespaces and table metadata in-memory. * - * Each NamespaceContainer represents a namespace level and can contain properties, + * Each InMemoryNamespace represents a namespace level and can contain properties, * tables, and child namespaces. This structure enables a tree-like representation * of nested namespaces. */ -class ICEBERG_EXPORT NamespaceContainer { +class ICEBERG_EXPORT InMemoryNamespace { public: /** * \brief Checks whether the given namespace exists. @@ -177,9 +177,9 @@ class ICEBERG_EXPORT NamespaceContainer { std::optional GetTableMetadataLocation( TableIdentifier const& table_ident) const; - template - static ContainerPtr GetNamespaceContainerImpl(ContainerPtr root, - const Namespace& namespace_ident) { + template + static NamespacePtr GetNamespaceImpl(NamespacePtr root, + const Namespace& namespace_ident) { auto node = root; for (const auto& part_level : namespace_ident.levels) { auto it = node->children_.find(part_level); @@ -192,8 +192,8 @@ class ICEBERG_EXPORT NamespaceContainer { } private: - /// Map of child namespace names to their corresponding container instances. - std::unordered_map children_; + /// Map of child namespace names to their corresponding namespace instances. + std::unordered_map children_; /// Key-value property map for this namespace. std::unordered_map properties_; diff --git a/test/memory_catalog_test.cc b/test/memory_catalog_test.cc index 9b09a60bf..ce5b42ce2 100644 --- a/test/memory_catalog_test.cc +++ b/test/memory_catalog_test.cc @@ -24,9 +24,9 @@ namespace iceberg { -class NamespaceContainerTest : public ::testing::Test { +class InMemoryNamespaceTest : public ::testing::Test { protected: - NamespaceContainer container; + InMemoryNamespace root_namespace_; static Namespace MakeNs(std::vector levels) { return Namespace{std::move(levels)}; @@ -38,92 +38,92 @@ class NamespaceContainerTest : public ::testing::Test { } }; -TEST_F(NamespaceContainerTest, CreateAndExistsNamespace) { - Namespace ns = MakeNs({"a", "b"}); - EXPECT_FALSE(container.NamespaceExists(ns)); - EXPECT_TRUE(container.CreateNamespace(ns, {{"k", "v"}})); - EXPECT_TRUE(container.NamespaceExists(ns)); +TEST_F(InMemoryNamespaceTest, CreateAndExistsNamespace) { + const auto ns = MakeNs({"a", "b"}); + EXPECT_FALSE(root_namespace_.NamespaceExists(ns)); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {{"k", "v"}})); + EXPECT_TRUE(root_namespace_.NamespaceExists(ns)); } -TEST_F(NamespaceContainerTest, CreateNamespaceTwiceFails) { - Namespace ns = MakeNs({"x", "y"}); - EXPECT_TRUE(container.CreateNamespace(ns, {{"a", "b"}})); - EXPECT_FALSE(container.CreateNamespace(ns, {{"a", "b"}})); +TEST_F(InMemoryNamespaceTest, CreateNamespaceTwiceFails) { + const auto ns = MakeNs({"x", "y"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {{"a", "b"}})); + EXPECT_FALSE(root_namespace_.CreateNamespace(ns, {{"a", "b"}})); } -TEST_F(NamespaceContainerTest, DeleteEmptyNamespaceSucceeds) { - Namespace ns = MakeNs({"del", "me"}); - EXPECT_TRUE(container.CreateNamespace(ns, {})); - EXPECT_TRUE(container.DeleteNamespace(ns)); - EXPECT_FALSE(container.NamespaceExists(ns)); +TEST_F(InMemoryNamespaceTest, DeleteEmptyNamespaceSucceeds) { + const auto ns = MakeNs({"del", "me"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); + EXPECT_TRUE(root_namespace_.DeleteNamespace(ns)); + EXPECT_FALSE(root_namespace_.NamespaceExists(ns)); } -TEST_F(NamespaceContainerTest, DeleteNonEmptyNamespaceFails) { - Namespace ns = MakeNs({"del", "fail"}); - EXPECT_TRUE(container.CreateNamespace(ns, {})); - auto table = MakeTable({"del", "fail"}, "t1"); - EXPECT_TRUE(container.RegisterTable(table, "loc1")); - EXPECT_FALSE(container.DeleteNamespace(ns)); +TEST_F(InMemoryNamespaceTest, DeleteNonEmptyNamespaceFails) { + const auto ns = MakeNs({"del", "fail"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); + const auto table = MakeTable({"del", "fail"}, "t1"); + EXPECT_TRUE(root_namespace_.RegisterTable(table, "loc1")); + EXPECT_FALSE(root_namespace_.DeleteNamespace(ns)); } -TEST_F(NamespaceContainerTest, ReplaceAndGetProperties) { - Namespace ns = MakeNs({"props"}); - EXPECT_TRUE(container.CreateNamespace(ns, {{"x", "1"}})); - EXPECT_TRUE(container.ReplaceProperties(ns, {{"x", "2"}, {"y", "3"}})); +TEST_F(InMemoryNamespaceTest, ReplaceAndGetProperties) { + const auto ns = MakeNs({"props"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {{"x", "1"}})); + EXPECT_TRUE(root_namespace_.ReplaceProperties(ns, {{"x", "2"}, {"y", "3"}})); - auto props = container.GetProperties(ns); + const auto props = root_namespace_.GetProperties(ns); ASSERT_TRUE(props.has_value()); EXPECT_EQ(props->at("x"), "2"); EXPECT_EQ(props->at("y"), "3"); } -TEST_F(NamespaceContainerTest, GetPropertiesNonExistReturnsNullopt) { - Namespace ns = MakeNs({"unknown"}); - EXPECT_FALSE(container.GetProperties(ns).has_value()); +TEST_F(InMemoryNamespaceTest, GetPropertiesNonExistReturnsNullopt) { + const auto ns = MakeNs({"unknown"}); + EXPECT_FALSE(root_namespace_.GetProperties(ns).has_value()); } -TEST_F(NamespaceContainerTest, RegisterAndLookupTable) { - Namespace ns = MakeNs({"tbl", "ns"}); - EXPECT_TRUE(container.CreateNamespace(ns, {})); - auto t1 = MakeTable(ns.levels, "table1"); - EXPECT_TRUE(container.RegisterTable(t1, "metadata/path/1")); +TEST_F(InMemoryNamespaceTest, RegisterAndLookupTable) { + const auto ns = MakeNs({"tbl", "ns"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); + const auto t1 = MakeTable(ns.levels, "table1"); + EXPECT_TRUE(root_namespace_.RegisterTable(t1, "metadata/path/1")); - EXPECT_TRUE(container.TableExists(t1)); - auto location = container.GetTableMetadataLocation(t1); + EXPECT_TRUE(root_namespace_.TableExists(t1)); + const auto location = root_namespace_.GetTableMetadataLocation(t1); ASSERT_TRUE(location.has_value()); EXPECT_EQ(location.value(), "metadata/path/1"); } -TEST_F(NamespaceContainerTest, UnregisterTable) { - Namespace ns = MakeNs({"tbl", "unreg"}); - EXPECT_TRUE(container.CreateNamespace(ns, {})); - auto t = MakeTable(ns.levels, "t2"); - EXPECT_TRUE(container.RegisterTable(t, "meta2")); - EXPECT_TRUE(container.UnregisterTable(t)); - EXPECT_FALSE(container.TableExists(t)); +TEST_F(InMemoryNamespaceTest, UnregisterTable) { + const auto ns = MakeNs({"tbl", "unreg"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); + const auto t = MakeTable(ns.levels, "t2"); + EXPECT_TRUE(root_namespace_.RegisterTable(t, "meta2")); + EXPECT_TRUE(root_namespace_.UnregisterTable(t)); + EXPECT_FALSE(root_namespace_.TableExists(t)); } -TEST_F(NamespaceContainerTest, RegisterTableOnNonExistingNamespaceFails) { - auto t = MakeTable({"nonexist", "ns"}, "oops"); - EXPECT_FALSE(container.RegisterTable(t, "x")); +TEST_F(InMemoryNamespaceTest, RegisterTableOnNonExistingNamespaceFails) { + const auto t = MakeTable({"nonexist", "ns"}, "oops"); + EXPECT_FALSE(root_namespace_.RegisterTable(t, "x")); } -TEST_F(NamespaceContainerTest, ListChildrenNamespaces) { - EXPECT_TRUE(container.CreateNamespace(MakeNs({"a"}), {})); - EXPECT_TRUE(container.CreateNamespace(MakeNs({"a", "b"}), {})); - EXPECT_TRUE(container.CreateNamespace(MakeNs({"a", "c"}), {})); - auto children = container.ListChildrenNamespaces(MakeNs({"a"})); +TEST_F(InMemoryNamespaceTest, ListChildrenNamespaces) { + EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a"}), {})); + EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a", "b"}), {})); + EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a", "c"}), {})); + const auto children = root_namespace_.ListChildrenNamespaces(MakeNs({"a"})); EXPECT_THAT(children, ::testing::UnorderedElementsAre("b", "c")); } -TEST_F(NamespaceContainerTest, ListTablesReturnsCorrectNames) { - Namespace ns = MakeNs({"list", "tables"}); - EXPECT_TRUE(container.CreateNamespace(ns, {})); - EXPECT_TRUE(container.RegisterTable(MakeTable(ns.levels, "a"), "loc_a")); - EXPECT_TRUE(container.RegisterTable(MakeTable(ns.levels, "b"), "loc_b")); - EXPECT_TRUE(container.RegisterTable(MakeTable(ns.levels, "c"), "loc_c")); +TEST_F(InMemoryNamespaceTest, ListTablesReturnsCorrectNames) { + const auto ns = MakeNs({"list", "tables"}); + EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); + EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "a"), "loc_a")); + EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "b"), "loc_b")); + EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "c"), "loc_c")); - auto tables = container.ListTables(ns); + auto tables = root_namespace_.ListTables(ns); EXPECT_THAT(tables, ::testing::UnorderedElementsAre("a", "b", "c")); } @@ -131,12 +131,12 @@ class MemoryCatalogTest : public ::testing::Test { protected: void SetUp() override { file_io_ = nullptr; // TODO(Guotao): A real FileIO instance needs to be constructed. - catalog_ = std::make_unique(file_io_, "/tmp/warehouse/"); + catalog_ = std::make_unique(file_io_, "/tmp/warehouse/"); catalog_->Initialize("test_catalog", {{"prop1", "val1"}}); } std::shared_ptr file_io_; - std::unique_ptr catalog_; + std::unique_ptr catalog_; }; TEST_F(MemoryCatalogTest, NameAndInitialization) { From 38b85c0b98b4b78a19bd1dc41d3e307ab9469bc1 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Thu, 24 Apr 2025 09:59:39 +0800 Subject: [PATCH 4/9] Change the return result type of the method to use Result/Status --- src/iceberg/catalog.h | 13 ++- src/iceberg/catalog/memory_catalog.cc | 126 +++++++++++++++----------- src/iceberg/catalog/memory_catalog.h | 64 ++++++------- src/iceberg/result.h | 6 +- test/memory_catalog_test.cc | 6 +- 5 files changed, 122 insertions(+), 93 deletions(-) diff --git a/src/iceberg/catalog.h b/src/iceberg/catalog.h index 121b0da36..0cf4de859 100644 --- a/src/iceberg/catalog.h +++ b/src/iceberg/catalog.h @@ -90,8 +90,11 @@ class ICEBERG_EXPORT Catalog { /// \brief Check whether table exists /// /// \param identifier a table identifier - /// \return true if the table exists, false otherwise - virtual bool TableExists(const TableIdentifier& identifier) const = 0; + /// \return Status indicating success or failure. + /// - On success, the table existence was successfully checked (actual existence + /// may be inferred elsewhere). + /// - On failure, contains error information. + virtual Status TableExists(const TableIdentifier& identifier) const = 0; /// \brief Drop a table; optionally delete data and metadata files /// @@ -100,8 +103,10 @@ class ICEBERG_EXPORT Catalog { /// /// \param identifier a table identifier /// \param purge if true, delete all data and metadata files in the table - /// \return true if the table was dropped, false if the table did not exist - virtual bool DropTable(const TableIdentifier& identifier, bool purge) = 0; + /// \return Status indicating the outcome of the operation. + /// - On success, the table was dropped (or did not exist). + /// - On failure, contains error information. + virtual Status DropTable(const TableIdentifier& identifier, bool purge) = 0; /// \brief Load a table /// diff --git a/src/iceberg/catalog/memory_catalog.cc b/src/iceberg/catalog/memory_catalog.cc index 3c354976a..d1707638f 100644 --- a/src/iceberg/catalog/memory_catalog.cc +++ b/src/iceberg/catalog/memory_catalog.cc @@ -24,18 +24,19 @@ #include "iceberg/exception.h" #include "iceberg/table.h" +#include "iceberg/util/macros.h" namespace iceberg { namespace { -InMemoryNamespace* GetNamespace(InMemoryNamespace* root, - const Namespace& namespace_ident) { +Result GetNamespace(InMemoryNamespace* root, + const Namespace& namespace_ident) { return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); } -const InMemoryNamespace* GetNamespace(const InMemoryNamespace* root, - const Namespace& namespace_ident) { +Result GetNamespace(const InMemoryNamespace* root, + const Namespace& namespace_ident) { return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); } @@ -60,10 +61,11 @@ Result> InMemoryCatalog::ListTables( const Namespace& ns) const { std::unique_lock lock(mutex_); const auto& table_names = root_namespace_->ListTables(ns); + ICEBERG_RETURN_UNEXPECTED(table_names); std::vector table_idents; - table_idents.reserve(table_names.size()); + table_idents.reserve(table_names.value().size()); std::ranges::transform( - table_names, std::back_inserter(table_idents), + table_names.value(), std::back_inserter(table_idents), [&ns](auto const& table_name) { return TableIdentifier(ns, table_name); }); return table_idents; } @@ -92,12 +94,12 @@ Result> InMemoryCatalog::StageCreateTable( {.kind = ErrorKind::kNotImplemented, .message = "StageCreateTable"}); } -bool InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { +Status InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { std::unique_lock lock(mutex_); return root_namespace_->TableExists(identifier); } -bool InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { +Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { std::unique_lock lock(mutex_); // TODO(Guotao): Delete all metadata files if purge is true. return root_namespace_->UnregisterTable(identifier); @@ -127,16 +129,19 @@ std::unique_ptr InMemoryCatalog::BuildTable( throw IcebergError("not implemented"); } -bool InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const { - return GetNamespace(this, namespace_ident) != nullptr; +Status InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const { + const auto ns = GetNamespace(this, namespace_ident); + ICEBERG_RETURN_UNEXPECTED(ns); + return {}; } -std::vector InMemoryNamespace::ListChildrenNamespaces( +Result> InMemoryNamespace::ListChildrenNamespaces( const std::optional& parent_namespace_ident) const { auto ns = this; if (parent_namespace_ident.has_value()) { - ns = GetNamespace(this, *parent_namespace_ident); - if (!ns) return {}; + const auto nsRs = GetNamespace(this, *parent_namespace_ident); + ICEBERG_RETURN_UNEXPECTED(nsRs); + ns = *nsRs; } std::vector names; @@ -147,12 +152,15 @@ std::vector InMemoryNamespace::ListChildrenNamespaces( return names; } -bool InMemoryNamespace::CreateNamespace( +Status InMemoryNamespace::CreateNamespace( const Namespace& namespace_ident, const std::unordered_map& properties) { + if (namespace_ident.levels.empty()) { + return InvalidArgument("namespace identifier is empty"); + } + auto ns = this; bool newly_created = false; - for (const auto& part_level : namespace_ident.levels) { if (auto it = ns->children_.find(part_level); it == ns->children_.end()) { ns = &ns->children_[part_level]; @@ -161,58 +169,62 @@ bool InMemoryNamespace::CreateNamespace( ns = &it->second; } } - if (!newly_created) { - return false; + return AlreadyExists("{}", namespace_ident.levels.back()); } ns->properties_ = properties; - return true; + return {}; } -bool InMemoryNamespace::DeleteNamespace(const Namespace& namespace_ident) { - if (namespace_ident.levels.empty()) return false; +Status InMemoryNamespace::DeleteNamespace(const Namespace& namespace_ident) { + if (namespace_ident.levels.empty()) { + return InvalidArgument("namespace identifier is empty"); + } auto parent_namespace_ident = namespace_ident; const auto to_delete = parent_namespace_ident.levels.back(); parent_namespace_ident.levels.pop_back(); - auto* parent = GetNamespace(this, parent_namespace_ident); - if (!parent) return false; + const auto parentRs = GetNamespace(this, parent_namespace_ident); + ICEBERG_RETURN_UNEXPECTED(parentRs); - auto it = parent->children_.find(to_delete); - if (it == parent->children_.end()) return false; + const auto it = parentRs.value()->children_.find(to_delete); + if (it == parentRs.value()->children_.end()) { + return NotFound("namespace {} is not found", to_delete); + } const auto& target = it->second; if (!target.children_.empty() || !target.table_metadata_locations_.empty()) { - return false; + return NotAllowed("{} has other sub-namespaces and cannot be deleted", to_delete); } - return parent->children_.erase(to_delete) > 0; + parentRs.value()->children_.erase(to_delete); + return {}; } -std::optional> -InMemoryNamespace::GetProperties(const Namespace& namespace_ident) const { +Result> InMemoryNamespace::GetProperties( + const Namespace& namespace_ident) const { const auto ns = GetNamespace(this, namespace_ident); - if (!ns) return std::nullopt; - return ns->properties_; + ICEBERG_RETURN_UNEXPECTED(ns); + return ns.value()->properties_; } -bool InMemoryNamespace::ReplaceProperties( +Status InMemoryNamespace::ReplaceProperties( const Namespace& namespace_ident, const std::unordered_map& properties) { const auto ns = GetNamespace(this, namespace_ident); - if (!ns) return false; - ns->properties_ = properties; - return true; + ICEBERG_RETURN_UNEXPECTED(ns); + ns.value()->properties_ = properties; + return {}; } -std::vector InMemoryNamespace::ListTables( +Result> InMemoryNamespace::ListTables( const Namespace& namespace_ident) const { const auto ns = GetNamespace(this, namespace_ident); - if (!ns) return {}; + ICEBERG_RETURN_UNEXPECTED(ns); - const auto& locations = ns->table_metadata_locations_; + const auto& locations = ns.value()->table_metadata_locations_; std::vector table_names; table_names.reserve(locations.size()); @@ -223,33 +235,41 @@ std::vector InMemoryNamespace::ListTables( return table_names; } -bool InMemoryNamespace::RegisterTable(TableIdentifier const& table_ident, - const std::string& metadata_location) { +Status InMemoryNamespace::RegisterTable(TableIdentifier const& table_ident, + const std::string& metadata_location) { const auto ns = GetNamespace(this, table_ident.ns); - if (!ns) return false; - if (ns->table_metadata_locations_.contains(table_ident.name)) return false; - ns->table_metadata_locations_[table_ident.name] = metadata_location; - return true; + ICEBERG_RETURN_UNEXPECTED(ns); + if (ns.value()->table_metadata_locations_.contains(table_ident.name)) { + return AlreadyExists("{} already exists", table_ident.name); + } + ns.value()->table_metadata_locations_[table_ident.name] = metadata_location; + return {}; } -bool InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) { +Status InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) { const auto ns = GetNamespace(this, table_ident.ns); - if (!ns) return false; - return ns->table_metadata_locations_.erase(table_ident.name) > 0; + ICEBERG_RETURN_UNEXPECTED(ns); + ns.value()->table_metadata_locations_.erase(table_ident.name); + return {}; } -bool InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const { +Status InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const { const auto ns = GetNamespace(this, table_ident.ns); - if (!ns) return false; - return ns->table_metadata_locations_.contains(table_ident.name); + ICEBERG_RETURN_UNEXPECTED(ns); + if (!ns.value()->table_metadata_locations_.contains(table_ident.name)) { + return NotFound("{} does not exist", table_ident.name); + } + return {}; } -std::optional InMemoryNamespace::GetTableMetadataLocation( +Result InMemoryNamespace::GetTableMetadataLocation( TableIdentifier const& table_ident) const { const auto ns = GetNamespace(this, table_ident.ns); - if (!ns) return std::nullopt; - const auto it = ns->table_metadata_locations_.find(table_ident.name); - if (it == ns->table_metadata_locations_.end()) return std::nullopt; + ICEBERG_RETURN_UNEXPECTED(ns); + const auto it = ns.value()->table_metadata_locations_.find(table_ident.name); + if (it == ns.value()->table_metadata_locations_.end()) { + return NotFound("{} does not exist", table_ident.name); + } return it->second; } } // namespace iceberg diff --git a/src/iceberg/catalog/memory_catalog.h b/src/iceberg/catalog/memory_catalog.h index eed075f1f..47f01a7b8 100644 --- a/src/iceberg/catalog/memory_catalog.h +++ b/src/iceberg/catalog/memory_catalog.h @@ -56,9 +56,9 @@ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { const std::string& location, const std::unordered_map& properties) override; - bool TableExists(const TableIdentifier& identifier) const override; + Status TableExists(const TableIdentifier& identifier) const override; - bool DropTable(const TableIdentifier& identifier, bool purge) override; + Status DropTable(const TableIdentifier& identifier, bool purge) override; Result> LoadTable( const TableIdentifier& identifier) const override; @@ -91,9 +91,9 @@ class ICEBERG_EXPORT InMemoryNamespace { /** * \brief Checks whether the given namespace exists. * \param[in] namespace_ident The namespace to check. - * \return True if the namespace exists; false otherwise. + * \return Status indicating success or failure. */ - bool NamespaceExists(const Namespace& namespace_ident) const; + Status NamespaceExists(const Namespace& namespace_ident) const; /** * \brief Lists immediate child namespaces under the given parent namespace. @@ -101,90 +101,90 @@ class ICEBERG_EXPORT InMemoryNamespace { * the children of the root are returned. * \return A vector of child namespace names. */ - std::vector ListChildrenNamespaces( + Result> ListChildrenNamespaces( const std::optional& parent_namespace_ident = std::nullopt) const; /** * \brief Creates a new namespace with the specified properties. * \param[in] namespace_ident The namespace to create. * \param[in] properties A map of key-value pairs to associate with the namespace. - * \return True if the namespace was successfully created; false if it already exists. + * \return Status indicating success or failure. */ - bool CreateNamespace(const Namespace& namespace_ident, - const std::unordered_map& properties); + Status CreateNamespace(const Namespace& namespace_ident, + const std::unordered_map& properties); /** * \brief Deletes an existing namespace. * \param[in] namespace_ident The namespace to delete. - * \return True if the namespace was successfully deleted; false if it does not exist. + * \return Status indicating success or failure. */ - bool DeleteNamespace(const Namespace& namespace_ident); + Status DeleteNamespace(const Namespace& namespace_ident); /** * \brief Retrieves the properties of the specified namespace. * \param[in] namespace_ident The namespace whose properties to retrieve. - * \return An optional containing the properties map if the namespace exists; - * std::nullopt otherwise. + * \return An containing the properties map if the namespace exists; + * Errpr otherwise. */ - std::optional> GetProperties( + Result> GetProperties( const Namespace& namespace_ident) const; /** * \brief Replaces all properties of the given namespace. * \param[in] namespace_ident The namespace whose properties will be replaced. * \param[in] properties The new properties map. - * \return True if the namespace exists and properties were replaced; false otherwise. + * \return Status indicating success or failure. */ - bool ReplaceProperties(const Namespace& namespace_ident, - const std::unordered_map& properties); + Status ReplaceProperties( + const Namespace& namespace_ident, + const std::unordered_map& properties); /** * \brief Lists all table names under the specified namespace. * \param[in] namespace_ident The namespace from which to list tables. - * \return A vector of table names. + * \return A vector of table names or error. */ - std::vector ListTables(const Namespace& namespace_ident) const; + Result> ListTables(const Namespace& namespace_ident) const; /** * \brief Registers a table in the given namespace with a metadata location. * \param[in] table_ident The fully qualified identifier of the table. * \param[in] metadata_location The path to the table's metadata. - * \return True if the table was registered successfully; false otherwise. + * \return Status indicating success or failure. */ - bool RegisterTable(TableIdentifier const& table_ident, - const std::string& metadata_location); + Status RegisterTable(TableIdentifier const& table_ident, + const std::string& metadata_location); /** * \brief Unregisters a table from the specified namespace. * \param[in] table_ident The identifier of the table to unregister. - * \return True if the table existed and was removed; false otherwise. + * \return Status indicating success or failure. */ - bool UnregisterTable(TableIdentifier const& table_ident); + Status UnregisterTable(TableIdentifier const& table_ident); /** * \brief Checks if a table exists in the specified namespace. * \param[in] table_ident The identifier of the table to check. - * \return True if the table exists; false otherwise. + * \return Status indicating success or failure. */ - bool TableExists(TableIdentifier const& table_ident) const; + Status TableExists(TableIdentifier const& table_ident) const; /** * \brief Gets the metadata location for the specified table. * \param[in] table_ident The identifier of the table. - * \return An optional string containing the metadata location if the table exists; - * std::nullopt otherwise. + * \return An string containing the metadata location if the table exists; + * Error otherwise. */ - std::optional GetTableMetadataLocation( - TableIdentifier const& table_ident) const; + Result GetTableMetadataLocation(TableIdentifier const& table_ident) const; template - static NamespacePtr GetNamespaceImpl(NamespacePtr root, - const Namespace& namespace_ident) { + static Result GetNamespaceImpl(NamespacePtr root, + const Namespace& namespace_ident) { auto node = root; for (const auto& part_level : namespace_ident.levels) { auto it = node->children_.find(part_level); if (it == node->children_.end()) { - return nullptr; + return NoSuchNamespace("{}", part_level); } node = &it->second; } diff --git a/src/iceberg/result.h b/src/iceberg/result.h index 38d9e381f..806453fea 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -38,6 +38,7 @@ enum class ErrorKind { kJsonParseError, kNoSuchNamespace, kNoSuchTable, + kNotAllowed, kNotFound, kNotImplemented, kNotSupported, @@ -65,8 +66,8 @@ using Status = Result; /// \brief Macro to define error creation functions #define DEFINE_ERROR_FUNCTION(name) \ template \ - inline auto name(const std::format_string fmt, Args&&... args) \ - -> unexpected { \ + inline auto name(const std::format_string fmt, \ + Args&&... args) -> unexpected { \ return unexpected( \ {ErrorKind::k##name, std::format(fmt, std::forward(args)...)}); \ } @@ -80,6 +81,7 @@ DEFINE_ERROR_FUNCTION(IOError) DEFINE_ERROR_FUNCTION(JsonParseError) DEFINE_ERROR_FUNCTION(NoSuchNamespace) DEFINE_ERROR_FUNCTION(NoSuchTable) +DEFINE_ERROR_FUNCTION(NotAllowed) DEFINE_ERROR_FUNCTION(NotFound) DEFINE_ERROR_FUNCTION(NotImplemented) DEFINE_ERROR_FUNCTION(NotSupported) diff --git a/test/memory_catalog_test.cc b/test/memory_catalog_test.cc index ce5b42ce2..8fb9e2b1d 100644 --- a/test/memory_catalog_test.cc +++ b/test/memory_catalog_test.cc @@ -113,7 +113,8 @@ TEST_F(InMemoryNamespaceTest, ListChildrenNamespaces) { EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a", "b"}), {})); EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a", "c"}), {})); const auto children = root_namespace_.ListChildrenNamespaces(MakeNs({"a"})); - EXPECT_THAT(children, ::testing::UnorderedElementsAre("b", "c")); + EXPECT_TRUE(children); + EXPECT_THAT(*children, ::testing::UnorderedElementsAre("b", "c")); } TEST_F(InMemoryNamespaceTest, ListTablesReturnsCorrectNames) { @@ -124,7 +125,8 @@ TEST_F(InMemoryNamespaceTest, ListTablesReturnsCorrectNames) { EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "c"), "loc_c")); auto tables = root_namespace_.ListTables(ns); - EXPECT_THAT(tables, ::testing::UnorderedElementsAre("a", "b", "c")); + EXPECT_TRUE(tables); + EXPECT_THAT(*tables, ::testing::UnorderedElementsAre("a", "b", "c")); } class MemoryCatalogTest : public ::testing::Test { From 7f3176057ee15e5525eaa59accc44db104a5eefc Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Thu, 24 Apr 2025 10:05:06 +0800 Subject: [PATCH 5/9] fix clang-format --- src/iceberg/result.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/iceberg/result.h b/src/iceberg/result.h index 806453fea..a682e8316 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -66,8 +66,8 @@ using Status = Result; /// \brief Macro to define error creation functions #define DEFINE_ERROR_FUNCTION(name) \ template \ - inline auto name(const std::format_string fmt, \ - Args&&... args) -> unexpected { \ + inline auto name(const std::format_string fmt, Args&&... args) \ + -> unexpected { \ return unexpected( \ {ErrorKind::k##name, std::format(fmt, std::forward(args)...)}); \ } From 17a6d9f641cefa7d0e0169aa3e03404898d5d69b Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Sat, 10 May 2025 14:01:32 +0800 Subject: [PATCH 6/9] rename memory catalog --- src/iceberg/CMakeLists.txt | 2 +- src/iceberg/catalog.h | 16 +------ ...memory_catalog.cc => in_memory_catalog.cc} | 44 +++++++------------ .../{memory_catalog.h => in_memory_catalog.h} | 18 +++----- test/CMakeLists.txt | 2 +- ...alog_test.cc => in_memory_catalog_test.cc} | 9 ++-- 6 files changed, 32 insertions(+), 59 deletions(-) rename src/iceberg/catalog/{memory_catalog.cc => in_memory_catalog.cc} (86%) rename src/iceberg/catalog/{memory_catalog.h => in_memory_catalog.h} (93%) rename test/{memory_catalog_test.cc => in_memory_catalog_test.cc} (93%) diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 121a2ffc0..146d94173 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -19,7 +19,7 @@ set(ICEBERG_INCLUDES "$" "$") set(ICEBERG_SOURCES arrow_c_data_internal.cc - catalog/memory_catalog.cc + catalog/in_memory_catalog.cc demo.cc expression/expression.cc file_reader.cc diff --git a/src/iceberg/catalog.h b/src/iceberg/catalog.h index 0cf4de859..b97d2733c 100644 --- a/src/iceberg/catalog.h +++ b/src/iceberg/catalog.h @@ -90,11 +90,11 @@ class ICEBERG_EXPORT Catalog { /// \brief Check whether table exists /// /// \param identifier a table identifier - /// \return Status indicating success or failure. + /// \return Result indicating table exists or not. /// - On success, the table existence was successfully checked (actual existence /// may be inferred elsewhere). /// - On failure, contains error information. - virtual Status TableExists(const TableIdentifier& identifier) const = 0; + virtual Result TableExists(const TableIdentifier& identifier) const = 0; /// \brief Drop a table; optionally delete data and metadata files /// @@ -124,18 +124,6 @@ class ICEBERG_EXPORT Catalog { virtual Result> RegisterTable( const TableIdentifier& identifier, const std::string& metadata_file_location) = 0; - /// \brief Initialize a catalog given a custom name and a map of catalog properties - /// - /// A custom Catalog implementation must have a default constructor. A compute engine - /// will first initialize the catalog without any arguments, and then call this method - /// to complete catalog initialization with properties passed into the engine. - /// - /// \param name a custom name for the catalog - /// \param properties catalog properties - virtual void Initialize( - const std::string& name, - const std::unordered_map& properties) = 0; - /// \brief Instantiate a builder to either create a table or start a create/replace /// transaction /// diff --git a/src/iceberg/catalog/memory_catalog.cc b/src/iceberg/catalog/in_memory_catalog.cc similarity index 86% rename from src/iceberg/catalog/memory_catalog.cc rename to src/iceberg/catalog/in_memory_catalog.cc index d1707638f..a8ced37b0 100644 --- a/src/iceberg/catalog/memory_catalog.cc +++ b/src/iceberg/catalog/in_memory_catalog.cc @@ -17,7 +17,7 @@ * under the License. */ -#include "iceberg/catalog/memory_catalog.h" +#include "iceberg/catalog/in_memory_catalog.h" #include #include // IWYU pragma: keep @@ -42,19 +42,15 @@ Result GetNamespace(const InMemoryNamespace* root, } // namespace -InMemoryCatalog::InMemoryCatalog(std::shared_ptr file_io, - std::string warehouse_location) - : file_io_(std::move(file_io)), +InMemoryCatalog::InMemoryCatalog(std::string name, std::shared_ptr file_io, + std::string warehouse_location, + std::unordered_map properties) + : catalog_name_(std::move(name)), + properties_(std::move(properties)), + file_io_(std::move(file_io)), warehouse_location_(std::move(warehouse_location)), root_namespace_(std::make_unique()) {} -void InMemoryCatalog::Initialize( - const std::string& name, - const std::unordered_map& properties) { - catalog_name_ = name; - properties_ = properties; -} - std::string_view InMemoryCatalog::name() const { return catalog_name_; } Result> InMemoryCatalog::ListTables( @@ -74,27 +70,24 @@ Result> InMemoryCatalog::CreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, const std::unordered_map& properties) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "CreateTable"}); + return NotImplemented("create table"); } Result> InMemoryCatalog::UpdateTable( const TableIdentifier& identifier, const std::vector>& requirements, const std::vector>& updates) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "UpdateTable"}); + return NotImplemented("update table"); } Result> InMemoryCatalog::StageCreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, const std::unordered_map& properties) { - return unexpected( - {.kind = ErrorKind::kNotImplemented, .message = "StageCreateTable"}); + return NotImplemented("stage create table"); } -Status InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { +Result InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { std::unique_lock lock(mutex_); return root_namespace_->TableExists(identifier); } @@ -107,19 +100,17 @@ Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) Result> InMemoryCatalog::LoadTable( const TableIdentifier& identifier) const { - return unexpected({.kind = ErrorKind::kNotImplemented, .message = "LoadTable"}); + return NotImplemented("load table"); } Result> InMemoryCatalog::RegisterTable( const TableIdentifier& identifier, const std::string& metadata_file_location) { std::unique_lock lock(mutex_); if (!root_namespace_->NamespaceExists(identifier.ns)) { - return unexpected({.kind = ErrorKind::kNoSuchNamespace, - .message = "table namespace does not exist"}); + return NoSuchNamespace("table namespace does not exist."); } if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) { - return unexpected( - {.kind = ErrorKind::kUnknownError, .message = "The registry failed."}); + return UnknownError("The registry failed."); } return LoadTable(identifier); } @@ -253,13 +244,10 @@ Status InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) { return {}; } -Status InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const { +Result InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const { const auto ns = GetNamespace(this, table_ident.ns); ICEBERG_RETURN_UNEXPECTED(ns); - if (!ns.value()->table_metadata_locations_.contains(table_ident.name)) { - return NotFound("{} does not exist", table_ident.name); - } - return {}; + return ns.value()->table_metadata_locations_.contains(table_ident.name); } Result InMemoryNamespace::GetTableMetadataLocation( diff --git a/src/iceberg/catalog/memory_catalog.h b/src/iceberg/catalog/in_memory_catalog.h similarity index 93% rename from src/iceberg/catalog/memory_catalog.h rename to src/iceberg/catalog/in_memory_catalog.h index 47f01a7b8..68275b6f1 100644 --- a/src/iceberg/catalog/memory_catalog.h +++ b/src/iceberg/catalog/in_memory_catalog.h @@ -27,15 +27,11 @@ namespace iceberg { -class InMemoryNamespace; - class ICEBERG_EXPORT InMemoryCatalog : public Catalog { public: - InMemoryCatalog(std::shared_ptr file_io, std::string warehouse_location); - - void Initialize( - const std::string& name, - const std::unordered_map& properties) override; + InMemoryCatalog(std::string name, std::shared_ptr file_io, + std::string warehouse_location, + std::unordered_map properties); std::string_view name() const override; @@ -56,7 +52,7 @@ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { const std::string& location, const std::unordered_map& properties) override; - Status TableExists(const TableIdentifier& identifier) const override; + Result TableExists(const TableIdentifier& identifier) const override; Status DropTable(const TableIdentifier& identifier, bool purge) override; @@ -75,7 +71,7 @@ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { std::unordered_map properties_; std::shared_ptr file_io_; std::string warehouse_location_; - std::unique_ptr root_namespace_; + std::unique_ptr root_namespace_; mutable std::recursive_mutex mutex_; }; @@ -165,9 +161,9 @@ class ICEBERG_EXPORT InMemoryNamespace { /** * \brief Checks if a table exists in the specified namespace. * \param[in] table_ident The identifier of the table to check. - * \return Status indicating success or failure. + * \return Result indicating table exists or not. */ - Status TableExists(TableIdentifier const& table_ident) const; + Result TableExists(TableIdentifier const& table_ident) const; /** * \brief Gets the metadata location for the specified table. diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 00feb8556..8ca09bd16 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -44,7 +44,7 @@ target_link_libraries(schema_test PRIVATE iceberg_static GTest::gtest_main GTest add_test(NAME schema_test COMMAND schema_test) add_executable(catalog_test) -target_sources(catalog_test PRIVATE memory_catalog_test.cc) +target_sources(catalog_test PRIVATE in_memory_catalog_test.cc) target_link_libraries(catalog_test PRIVATE iceberg_static GTest::gtest_main GTest::gmock) add_test(NAME catalog_test COMMAND catalog_test) diff --git a/test/memory_catalog_test.cc b/test/in_memory_catalog_test.cc similarity index 93% rename from test/memory_catalog_test.cc rename to test/in_memory_catalog_test.cc index 8fb9e2b1d..321747079 100644 --- a/test/memory_catalog_test.cc +++ b/test/in_memory_catalog_test.cc @@ -17,7 +17,7 @@ * under the License. */ -#include "iceberg/catalog/memory_catalog.h" +#include "iceberg/catalog/in_memory_catalog.h" #include #include @@ -100,7 +100,7 @@ TEST_F(InMemoryNamespaceTest, UnregisterTable) { const auto t = MakeTable(ns.levels, "t2"); EXPECT_TRUE(root_namespace_.RegisterTable(t, "meta2")); EXPECT_TRUE(root_namespace_.UnregisterTable(t)); - EXPECT_FALSE(root_namespace_.TableExists(t)); + EXPECT_FALSE(root_namespace_.TableExists(t).value()); } TEST_F(InMemoryNamespaceTest, RegisterTableOnNonExistingNamespaceFails) { @@ -133,8 +133,9 @@ class MemoryCatalogTest : public ::testing::Test { protected: void SetUp() override { file_io_ = nullptr; // TODO(Guotao): A real FileIO instance needs to be constructed. - catalog_ = std::make_unique(file_io_, "/tmp/warehouse/"); - catalog_->Initialize("test_catalog", {{"prop1", "val1"}}); + std::unordered_map properties = {{"prop1", "val1"}}; + catalog_ = std::make_unique("test_catalog", file_io_, + "/tmp/warehouse/", properties); } std::shared_ptr file_io_; From a7a680f7c10183d25a49ef7244c0d72d290ff657 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Sat, 17 May 2025 14:08:25 +0800 Subject: [PATCH 7/9] Move the implementation details of memory catalog to cc --- src/iceberg/catalog/in_memory_catalog.cc | 393 ++++++++++++++++++----- src/iceberg/catalog/in_memory_catalog.h | 140 +------- test/in_memory_catalog_test.cc | 132 ++------ 3 files changed, 343 insertions(+), 322 deletions(-) diff --git a/src/iceberg/catalog/in_memory_catalog.cc b/src/iceberg/catalog/in_memory_catalog.cc index a8ced37b0..c187e1ece 100644 --- a/src/iceberg/catalog/in_memory_catalog.cc +++ b/src/iceberg/catalog/in_memory_catalog.cc @@ -21,6 +21,9 @@ #include #include // IWYU pragma: keep +#include +#include +#include #include "iceberg/exception.h" #include "iceberg/table.h" @@ -30,6 +33,129 @@ namespace iceberg { namespace { +/** + * \brief A hierarchical namespace that manages namespaces and table metadata in-memory. + * + * Each InMemoryNamespace represents a namespace level and can contain properties, + * tables, and child namespaces. This structure enables a tree-like representation + * of nested namespaces. + */ +class ICEBERG_EXPORT InMemoryNamespace { + public: + /** + * \brief Checks whether the given namespace exists. + * \param[in] namespace_ident The namespace to check. + * \return Status indicating success or failure. + */ + Status NamespaceExists(const Namespace& namespace_ident) const; + + /** + * \brief Lists immediate child namespaces under the given parent namespace. + * \param[in] parent_namespace_ident The optional parent namespace. If not provided, + * the children of the root are returned. + * \return A vector of child namespace names. + */ + Result> ListChildrenNamespaces( + const std::optional& parent_namespace_ident = std::nullopt) const; + + /** + * \brief Creates a new namespace with the specified properties. + * \param[in] namespace_ident The namespace to create. + * \param[in] properties A map of key-value pairs to associate with the namespace. + * \return Status indicating success or failure. + */ + Status CreateNamespace(const Namespace& namespace_ident, + const std::unordered_map& properties); + + /** + * \brief Deletes an existing namespace. + * \param[in] namespace_ident The namespace to delete. + * \return Status indicating success or failure. + */ + Status DeleteNamespace(const Namespace& namespace_ident); + + /** + * \brief Retrieves the properties of the specified namespace. + * \param[in] namespace_ident The namespace whose properties to retrieve. + * \return An containing the properties map if the namespace exists; + * Errpr otherwise. + */ + Result> GetProperties( + const Namespace& namespace_ident) const; + + /** + * \brief Replaces all properties of the given namespace. + * \param[in] namespace_ident The namespace whose properties will be replaced. + * \param[in] properties The new properties map. + * \return Status indicating success or failure. + */ + Status ReplaceProperties( + const Namespace& namespace_ident, + const std::unordered_map& properties); + + /** + * \brief Lists all table names under the specified namespace. + * \param[in] namespace_ident The namespace from which to list tables. + * \return A vector of table names or error. + */ + Result> ListTables(const Namespace& namespace_ident) const; + + /** + * \brief Registers a table in the given namespace with a metadata location. + * \param[in] table_ident The fully qualified identifier of the table. + * \param[in] metadata_location The path to the table's metadata. + * \return Status indicating success or failure. + */ + Status RegisterTable(TableIdentifier const& table_ident, + const std::string& metadata_location); + + /** + * \brief Unregisters a table from the specified namespace. + * \param[in] table_ident The identifier of the table to unregister. + * \return Status indicating success or failure. + */ + Status UnregisterTable(TableIdentifier const& table_ident); + + /** + * \brief Checks if a table exists in the specified namespace. + * \param[in] table_ident The identifier of the table to check. + * \return Result indicating table exists or not. + */ + Result TableExists(TableIdentifier const& table_ident) const; + + /** + * \brief Gets the metadata location for the specified table. + * \param[in] table_ident The identifier of the table. + * \return An string containing the metadata location if the table exists; + * Error otherwise. + */ + Result GetTableMetadataLocation(TableIdentifier const& table_ident) const; + + template + static Result GetNamespaceImpl(NamespacePtr root, + const Namespace& namespace_ident) { + auto node = root; + for (const auto& part_level : namespace_ident.levels) { + auto it = node->children_.find(part_level); + if (it == node->children_.end()) { + return NoSuchNamespace("{}", part_level); + } + node = &it->second; + } + return node; + } + + private: + /// Map of child namespace names to their corresponding namespace instances. + std::unordered_map children_; + + /// Key-value property map for this namespace. + std::unordered_map properties_; + + /// Mapping of table names to metadata file locations. + std::unordered_map table_metadata_locations_; +}; + Result GetNamespace(InMemoryNamespace* root, const Namespace& namespace_ident) { return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); @@ -40,86 +166,6 @@ Result GetNamespace(const InMemoryNamespace* root, return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); } -} // namespace - -InMemoryCatalog::InMemoryCatalog(std::string name, std::shared_ptr file_io, - std::string warehouse_location, - std::unordered_map properties) - : catalog_name_(std::move(name)), - properties_(std::move(properties)), - file_io_(std::move(file_io)), - warehouse_location_(std::move(warehouse_location)), - root_namespace_(std::make_unique()) {} - -std::string_view InMemoryCatalog::name() const { return catalog_name_; } - -Result> InMemoryCatalog::ListTables( - const Namespace& ns) const { - std::unique_lock lock(mutex_); - const auto& table_names = root_namespace_->ListTables(ns); - ICEBERG_RETURN_UNEXPECTED(table_names); - std::vector table_idents; - table_idents.reserve(table_names.value().size()); - std::ranges::transform( - table_names.value(), std::back_inserter(table_idents), - [&ns](auto const& table_name) { return TableIdentifier(ns, table_name); }); - return table_idents; -} - -Result> InMemoryCatalog::CreateTable( - const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, - const std::string& location, - const std::unordered_map& properties) { - return NotImplemented("create table"); -} - -Result> InMemoryCatalog::UpdateTable( - const TableIdentifier& identifier, - const std::vector>& requirements, - const std::vector>& updates) { - return NotImplemented("update table"); -} - -Result> InMemoryCatalog::StageCreateTable( - const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, - const std::string& location, - const std::unordered_map& properties) { - return NotImplemented("stage create table"); -} - -Result InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { - std::unique_lock lock(mutex_); - return root_namespace_->TableExists(identifier); -} - -Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { - std::unique_lock lock(mutex_); - // TODO(Guotao): Delete all metadata files if purge is true. - return root_namespace_->UnregisterTable(identifier); -} - -Result> InMemoryCatalog::LoadTable( - const TableIdentifier& identifier) const { - return NotImplemented("load table"); -} - -Result> InMemoryCatalog::RegisterTable( - const TableIdentifier& identifier, const std::string& metadata_file_location) { - std::unique_lock lock(mutex_); - if (!root_namespace_->NamespaceExists(identifier.ns)) { - return NoSuchNamespace("table namespace does not exist."); - } - if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) { - return UnknownError("The registry failed."); - } - return LoadTable(identifier); -} - -std::unique_ptr InMemoryCatalog::BuildTable( - const TableIdentifier& identifier, const Schema& schema) const { - throw IcebergError("not implemented"); -} - Status InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const { const auto ns = GetNamespace(this, namespace_ident); ICEBERG_RETURN_UNEXPECTED(ns); @@ -260,4 +306,191 @@ Result InMemoryNamespace::GetTableMetadataLocation( } return it->second; } + +} // namespace + +class ICEBERG_EXPORT InMemoryCatalogImpl { + public: + InMemoryCatalogImpl(std::string name, std::shared_ptr file_io, + std::string warehouse_location, + std::unordered_map properties); + + std::string_view name() const; + + Result> ListTables(const Namespace& ns) const; + + Result> CreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties); + + Result> UpdateTable( + const TableIdentifier& identifier, + const std::vector>& requirements, + const std::vector>& updates); + + Result> StageCreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties); + + Result TableExists(const TableIdentifier& identifier) const; + + Status DropTable(const TableIdentifier& identifier, bool purge); + + Result> LoadTable(const TableIdentifier& identifier) const; + + Result> RegisterTable(const TableIdentifier& identifier, + const std::string& metadata_file_location); + + std::unique_ptr BuildTable(const TableIdentifier& identifier, + const Schema& schema) const; + + private: + std::string catalog_name_; + std::unordered_map properties_; + std::shared_ptr file_io_; + std::string warehouse_location_; + std::unique_ptr root_namespace_; + mutable std::recursive_mutex mutex_; +}; + +InMemoryCatalogImpl::InMemoryCatalogImpl( + std::string name, std::shared_ptr file_io, std::string warehouse_location, + std::unordered_map properties) + : catalog_name_(std::move(name)), + properties_(std::move(properties)), + file_io_(std::move(file_io)), + warehouse_location_(std::move(warehouse_location)), + root_namespace_(std::make_unique()) {} + +std::string_view InMemoryCatalogImpl::name() const { return catalog_name_; } + +Result> InMemoryCatalogImpl::ListTables( + const Namespace& ns) const { + std::unique_lock lock(mutex_); + const auto& table_names = root_namespace_->ListTables(ns); + ICEBERG_RETURN_UNEXPECTED(table_names); + std::vector table_idents; + table_idents.reserve(table_names.value().size()); + std::ranges::transform( + table_names.value(), std::back_inserter(table_idents), + [&ns](auto const& table_name) { return TableIdentifier(ns, table_name); }); + return table_idents; +} + +Result> InMemoryCatalogImpl::CreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) { + return NotImplemented("create table"); +} + +Result> InMemoryCatalogImpl::UpdateTable( + const TableIdentifier& identifier, + const std::vector>& requirements, + const std::vector>& updates) { + return NotImplemented("update table"); +} + +Result> InMemoryCatalogImpl::StageCreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) { + return NotImplemented("stage create table"); +} + +Result InMemoryCatalogImpl::TableExists(const TableIdentifier& identifier) const { + std::unique_lock lock(mutex_); + return root_namespace_->TableExists(identifier); +} + +Status InMemoryCatalogImpl::DropTable(const TableIdentifier& identifier, bool purge) { + std::unique_lock lock(mutex_); + // TODO(Guotao): Delete all metadata files if purge is true. + return root_namespace_->UnregisterTable(identifier); +} + +Result> InMemoryCatalogImpl::LoadTable( + const TableIdentifier& identifier) const { + return NotImplemented("load table"); +} + +Result> InMemoryCatalogImpl::RegisterTable( + const TableIdentifier& identifier, const std::string& metadata_file_location) { + std::unique_lock lock(mutex_); + if (!root_namespace_->NamespaceExists(identifier.ns)) { + return NoSuchNamespace("table namespace does not exist."); + } + if (!root_namespace_->RegisterTable(identifier, metadata_file_location)) { + return UnknownError("The registry failed."); + } + return LoadTable(identifier); +} + +std::unique_ptr InMemoryCatalogImpl::BuildTable( + const TableIdentifier& identifier, const Schema& schema) const { + throw IcebergError("not implemented"); +} + +InMemoryCatalog::InMemoryCatalog( + std::string const& name, std::shared_ptr const& file_io, + std::string const& warehouse_location, + std::unordered_map const& properties) + : impl_(std::make_unique(name, file_io, warehouse_location, + properties)) {} + +InMemoryCatalog::~InMemoryCatalog() = default; + +std::string_view InMemoryCatalog::name() const { return impl_->name(); } + +Result> InMemoryCatalog::ListTables( + const Namespace& ns) const { + return impl_->ListTables(ns); +} + +Result> InMemoryCatalog::CreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) { + return impl_->CreateTable(identifier, schema, spec, location, properties); +} + +Result> InMemoryCatalog::UpdateTable( + const TableIdentifier& identifier, + const std::vector>& requirements, + const std::vector>& updates) { + return impl_->UpdateTable(identifier, requirements, updates); +} + +Result> InMemoryCatalog::StageCreateTable( + const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, + const std::string& location, + const std::unordered_map& properties) { + return impl_->StageCreateTable(identifier, schema, spec, location, properties); +} + +Result InMemoryCatalog::TableExists(const TableIdentifier& identifier) const { + return impl_->TableExists(identifier); +} + +Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { + return impl_->DropTable(identifier, purge); +} + +Result> InMemoryCatalog::LoadTable( + const TableIdentifier& identifier) const { + return impl_->LoadTable(identifier); +} + +Result> InMemoryCatalog::RegisterTable( + const TableIdentifier& identifier, const std::string& metadata_file_location) { + return impl_->RegisterTable(identifier, metadata_file_location); +} + +std::unique_ptr InMemoryCatalog::BuildTable( + const TableIdentifier& identifier, const Schema& schema) const { + throw IcebergError("not implemented"); +} + } // namespace iceberg diff --git a/src/iceberg/catalog/in_memory_catalog.h b/src/iceberg/catalog/in_memory_catalog.h index 68275b6f1..6063852a8 100644 --- a/src/iceberg/catalog/in_memory_catalog.h +++ b/src/iceberg/catalog/in_memory_catalog.h @@ -19,19 +19,16 @@ #pragma once -#include -#include -#include - #include "iceberg/catalog.h" namespace iceberg { class ICEBERG_EXPORT InMemoryCatalog : public Catalog { public: - InMemoryCatalog(std::string name, std::shared_ptr file_io, - std::string warehouse_location, - std::unordered_map properties); + InMemoryCatalog(std::string const& name, std::shared_ptr const& file_io, + std::string const& warehouse_location, + std::unordered_map const& properties); + ~InMemoryCatalog() override; std::string_view name() const override; @@ -67,134 +64,7 @@ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { const Schema& schema) const override; private: - std::string catalog_name_; - std::unordered_map properties_; - std::shared_ptr file_io_; - std::string warehouse_location_; - std::unique_ptr root_namespace_; - mutable std::recursive_mutex mutex_; + std::unique_ptr impl_; }; -/** - * \brief A hierarchical namespace that manages namespaces and table metadata in-memory. - * - * Each InMemoryNamespace represents a namespace level and can contain properties, - * tables, and child namespaces. This structure enables a tree-like representation - * of nested namespaces. - */ -class ICEBERG_EXPORT InMemoryNamespace { - public: - /** - * \brief Checks whether the given namespace exists. - * \param[in] namespace_ident The namespace to check. - * \return Status indicating success or failure. - */ - Status NamespaceExists(const Namespace& namespace_ident) const; - - /** - * \brief Lists immediate child namespaces under the given parent namespace. - * \param[in] parent_namespace_ident The optional parent namespace. If not provided, - * the children of the root are returned. - * \return A vector of child namespace names. - */ - Result> ListChildrenNamespaces( - const std::optional& parent_namespace_ident = std::nullopt) const; - - /** - * \brief Creates a new namespace with the specified properties. - * \param[in] namespace_ident The namespace to create. - * \param[in] properties A map of key-value pairs to associate with the namespace. - * \return Status indicating success or failure. - */ - Status CreateNamespace(const Namespace& namespace_ident, - const std::unordered_map& properties); - - /** - * \brief Deletes an existing namespace. - * \param[in] namespace_ident The namespace to delete. - * \return Status indicating success or failure. - */ - Status DeleteNamespace(const Namespace& namespace_ident); - - /** - * \brief Retrieves the properties of the specified namespace. - * \param[in] namespace_ident The namespace whose properties to retrieve. - * \return An containing the properties map if the namespace exists; - * Errpr otherwise. - */ - Result> GetProperties( - const Namespace& namespace_ident) const; - - /** - * \brief Replaces all properties of the given namespace. - * \param[in] namespace_ident The namespace whose properties will be replaced. - * \param[in] properties The new properties map. - * \return Status indicating success or failure. - */ - Status ReplaceProperties( - const Namespace& namespace_ident, - const std::unordered_map& properties); - - /** - * \brief Lists all table names under the specified namespace. - * \param[in] namespace_ident The namespace from which to list tables. - * \return A vector of table names or error. - */ - Result> ListTables(const Namespace& namespace_ident) const; - - /** - * \brief Registers a table in the given namespace with a metadata location. - * \param[in] table_ident The fully qualified identifier of the table. - * \param[in] metadata_location The path to the table's metadata. - * \return Status indicating success or failure. - */ - Status RegisterTable(TableIdentifier const& table_ident, - const std::string& metadata_location); - - /** - * \brief Unregisters a table from the specified namespace. - * \param[in] table_ident The identifier of the table to unregister. - * \return Status indicating success or failure. - */ - Status UnregisterTable(TableIdentifier const& table_ident); - - /** - * \brief Checks if a table exists in the specified namespace. - * \param[in] table_ident The identifier of the table to check. - * \return Result indicating table exists or not. - */ - Result TableExists(TableIdentifier const& table_ident) const; - - /** - * \brief Gets the metadata location for the specified table. - * \param[in] table_ident The identifier of the table. - * \return An string containing the metadata location if the table exists; - * Error otherwise. - */ - Result GetTableMetadataLocation(TableIdentifier const& table_ident) const; - - template - static Result GetNamespaceImpl(NamespacePtr root, - const Namespace& namespace_ident) { - auto node = root; - for (const auto& part_level : namespace_ident.levels) { - auto it = node->children_.find(part_level); - if (it == node->children_.end()) { - return NoSuchNamespace("{}", part_level); - } - node = &it->second; - } - return node; - } - - private: - /// Map of child namespace names to their corresponding namespace instances. - std::unordered_map children_; - - /// Key-value property map for this namespace. - std::unordered_map properties_; - - /// Mapping of table names to metadata file locations. - std::unordered_map table_metadata_locations_; -}; } // namespace iceberg diff --git a/test/in_memory_catalog_test.cc b/test/in_memory_catalog_test.cc index 321747079..60a3b0024 100644 --- a/test/in_memory_catalog_test.cc +++ b/test/in_memory_catalog_test.cc @@ -22,114 +22,11 @@ #include #include -namespace iceberg { - -class InMemoryNamespaceTest : public ::testing::Test { - protected: - InMemoryNamespace root_namespace_; - - static Namespace MakeNs(std::vector levels) { - return Namespace{std::move(levels)}; - } - - static TableIdentifier MakeTable(const std::vector& ns, - const std::string& name) { - return TableIdentifier{.ns = Namespace{ns}, .name = name}; - } -}; - -TEST_F(InMemoryNamespaceTest, CreateAndExistsNamespace) { - const auto ns = MakeNs({"a", "b"}); - EXPECT_FALSE(root_namespace_.NamespaceExists(ns)); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {{"k", "v"}})); - EXPECT_TRUE(root_namespace_.NamespaceExists(ns)); -} +#include "matchers.h" -TEST_F(InMemoryNamespaceTest, CreateNamespaceTwiceFails) { - const auto ns = MakeNs({"x", "y"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {{"a", "b"}})); - EXPECT_FALSE(root_namespace_.CreateNamespace(ns, {{"a", "b"}})); -} - -TEST_F(InMemoryNamespaceTest, DeleteEmptyNamespaceSucceeds) { - const auto ns = MakeNs({"del", "me"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); - EXPECT_TRUE(root_namespace_.DeleteNamespace(ns)); - EXPECT_FALSE(root_namespace_.NamespaceExists(ns)); -} - -TEST_F(InMemoryNamespaceTest, DeleteNonEmptyNamespaceFails) { - const auto ns = MakeNs({"del", "fail"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); - const auto table = MakeTable({"del", "fail"}, "t1"); - EXPECT_TRUE(root_namespace_.RegisterTable(table, "loc1")); - EXPECT_FALSE(root_namespace_.DeleteNamespace(ns)); -} - -TEST_F(InMemoryNamespaceTest, ReplaceAndGetProperties) { - const auto ns = MakeNs({"props"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {{"x", "1"}})); - EXPECT_TRUE(root_namespace_.ReplaceProperties(ns, {{"x", "2"}, {"y", "3"}})); - - const auto props = root_namespace_.GetProperties(ns); - ASSERT_TRUE(props.has_value()); - EXPECT_EQ(props->at("x"), "2"); - EXPECT_EQ(props->at("y"), "3"); -} - -TEST_F(InMemoryNamespaceTest, GetPropertiesNonExistReturnsNullopt) { - const auto ns = MakeNs({"unknown"}); - EXPECT_FALSE(root_namespace_.GetProperties(ns).has_value()); -} - -TEST_F(InMemoryNamespaceTest, RegisterAndLookupTable) { - const auto ns = MakeNs({"tbl", "ns"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); - const auto t1 = MakeTable(ns.levels, "table1"); - EXPECT_TRUE(root_namespace_.RegisterTable(t1, "metadata/path/1")); - - EXPECT_TRUE(root_namespace_.TableExists(t1)); - const auto location = root_namespace_.GetTableMetadataLocation(t1); - ASSERT_TRUE(location.has_value()); - EXPECT_EQ(location.value(), "metadata/path/1"); -} - -TEST_F(InMemoryNamespaceTest, UnregisterTable) { - const auto ns = MakeNs({"tbl", "unreg"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); - const auto t = MakeTable(ns.levels, "t2"); - EXPECT_TRUE(root_namespace_.RegisterTable(t, "meta2")); - EXPECT_TRUE(root_namespace_.UnregisterTable(t)); - EXPECT_FALSE(root_namespace_.TableExists(t).value()); -} - -TEST_F(InMemoryNamespaceTest, RegisterTableOnNonExistingNamespaceFails) { - const auto t = MakeTable({"nonexist", "ns"}, "oops"); - EXPECT_FALSE(root_namespace_.RegisterTable(t, "x")); -} - -TEST_F(InMemoryNamespaceTest, ListChildrenNamespaces) { - EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a"}), {})); - EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a", "b"}), {})); - EXPECT_TRUE(root_namespace_.CreateNamespace(MakeNs({"a", "c"}), {})); - const auto children = root_namespace_.ListChildrenNamespaces(MakeNs({"a"})); - EXPECT_TRUE(children); - EXPECT_THAT(*children, ::testing::UnorderedElementsAre("b", "c")); -} - -TEST_F(InMemoryNamespaceTest, ListTablesReturnsCorrectNames) { - const auto ns = MakeNs({"list", "tables"}); - EXPECT_TRUE(root_namespace_.CreateNamespace(ns, {})); - EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "a"), "loc_a")); - EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "b"), "loc_b")); - EXPECT_TRUE(root_namespace_.RegisterTable(MakeTable(ns.levels, "c"), "loc_c")); - - auto tables = root_namespace_.ListTables(ns); - EXPECT_TRUE(tables); - EXPECT_THAT(*tables, ::testing::UnorderedElementsAre("a", "b", "c")); -} +namespace iceberg { -class MemoryCatalogTest : public ::testing::Test { +class InMemoryCatalogTest : public ::testing::Test { protected: void SetUp() override { file_io_ = nullptr; // TODO(Guotao): A real FileIO instance needs to be constructed. @@ -142,8 +39,29 @@ class MemoryCatalogTest : public ::testing::Test { std::unique_ptr catalog_; }; -TEST_F(MemoryCatalogTest, NameAndInitialization) { +TEST_F(InMemoryCatalogTest, CatalogName) { EXPECT_EQ(catalog_->name(), "test_catalog"); + auto tablesRs = catalog_->ListTables(Namespace{{}}); + EXPECT_THAT(tablesRs, IsOk()); + ASSERT_TRUE(tablesRs->empty()); +} + +TEST_F(InMemoryCatalogTest, ListTables) { + auto tablesRs = catalog_->ListTables(Namespace{{}}); + EXPECT_THAT(tablesRs, IsOk()); + ASSERT_TRUE(tablesRs->empty()); +} + +TEST_F(InMemoryCatalogTest, TableExists) { + TableIdentifier tableIdent{.ns = {}, .name = "t1"}; + auto result = catalog_->TableExists(tableIdent); + EXPECT_THAT(result, HasValue(::testing::Eq(false))); +} + +TEST_F(InMemoryCatalogTest, DropTable) { + TableIdentifier tableIdent{.ns = {}, .name = "t1"}; + auto result = catalog_->DropTable(tableIdent, false); + EXPECT_THAT(result, IsOk()); } } // namespace iceberg From 1831078bcf8d2cb72d282c1ac1c6871231aab448 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Thu, 22 May 2025 13:30:05 +0800 Subject: [PATCH 8/9] fix comments --- src/iceberg/catalog.h | 70 +++++++++- src/iceberg/catalog/in_memory_catalog.cc | 165 +++++++++++++++++++---- src/iceberg/catalog/in_memory_catalog.h | 31 ++++- test/in_memory_catalog_test.cc | 25 ++++ 4 files changed, 261 insertions(+), 30 deletions(-) diff --git a/src/iceberg/catalog.h b/src/iceberg/catalog.h index b97d2733c..b7a632bc6 100644 --- a/src/iceberg/catalog.h +++ b/src/iceberg/catalog.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "iceberg/result.h" @@ -42,6 +43,67 @@ class ICEBERG_EXPORT Catalog { /// \brief Return the name for this catalog virtual std::string_view name() const = 0; + /// \brief Create a namespace with associated properties. + /// + /// \param ns the namespace to create + /// \param properties a key-value map of metadata for the namespace + /// \return Status::OK if created successfully; + /// ErrorKind::kAlreadyExists if the namespace already exists; + /// ErrorKind::kNotSupported if the operation is not supported + virtual Status CreateNamespace( + const Namespace& ns, + const std::unordered_map& properties) = 0; + + /// \brief List child namespaces from the given namespace. + /// + /// \param ns the parent namespace + /// \return a list of child namespaces; + /// ErrorKind::kNoSuchNamespace if the given namespace does not exist + virtual Result> ListNamespaces(const Namespace& ns) const = 0; + + /// \brief Get metadata properties for a namespace. + /// + /// \param ns the namespace to look up + /// \return a key-value map of metadata properties; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist + virtual Result> GetNamespaceProperties( + const Namespace& ns) const = 0; + + /// \brief Drop a namespace. + /// + /// \param ns the namespace to drop + /// \return Status::OK if dropped successfully; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist; + /// ErrorKind::kNotAllowed if the namespace is not empty + virtual Status DropNamespace(const Namespace& ns) = 0; + + /// \brief Check whether the namespace exists. + /// + /// \param ns the namespace to check + /// \return true if the namespace exists, false otherwise + virtual Result NamespaceExists(const Namespace& ns) const = 0; + + /// \brief Set metadata properties on a namespace. + /// + /// \param ns the namespace to modify + /// \param properties the properties to set or update + /// \return Status::OK if updated successfully; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist; + /// ErrorKind::kNotSupported if the operation is not supported + virtual Status SetNamespaceProperties( + const Namespace& ns, + const std::unordered_map& properties) = 0; + + /// \brief Remove a set of metadata properties from a namespace. + /// + /// \param ns the namespace to modify + /// \param properties the set of property keys to remove + /// \return Status::OK if removed successfully; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist; + /// ErrorKind::kNotSupported if the operation is not supported + virtual Status RemoveNamespaceProperties( + const Namespace& ns, const std::unordered_set& properties) = 0; + /// \brief Return all the identifiers under this namespace /// /// \param ns a namespace @@ -80,8 +142,8 @@ class ICEBERG_EXPORT Catalog { /// \param spec a partition spec /// \param location a location for the table; leave empty if unspecified /// \param properties a string map of table properties - /// \return a Transaction to create the table or ErrorKind::kAlreadyExists if the table - /// already exists + /// \return a Transaction to create the table or ErrorKind::kAlreadyExists if the + /// table already exists virtual Result> StageCreateTable( const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec, const std::string& location, @@ -91,8 +153,8 @@ class ICEBERG_EXPORT Catalog { /// /// \param identifier a table identifier /// \return Result indicating table exists or not. - /// - On success, the table existence was successfully checked (actual existence - /// may be inferred elsewhere). + /// - On success, the table existence was successfully checked (actual + /// existence may be inferred elsewhere). /// - On failure, contains error information. virtual Result TableExists(const TableIdentifier& identifier) const = 0; diff --git a/src/iceberg/catalog/in_memory_catalog.cc b/src/iceberg/catalog/in_memory_catalog.cc index c187e1ece..7175be4ca 100644 --- a/src/iceberg/catalog/in_memory_catalog.cc +++ b/src/iceberg/catalog/in_memory_catalog.cc @@ -47,16 +47,15 @@ class ICEBERG_EXPORT InMemoryNamespace { * \param[in] namespace_ident The namespace to check. * \return Status indicating success or failure. */ - Status NamespaceExists(const Namespace& namespace_ident) const; + Result NamespaceExists(const Namespace& namespace_ident) const; /** * \brief Lists immediate child namespaces under the given parent namespace. - * \param[in] parent_namespace_ident The optional parent namespace. If not provided, - * the children of the root are returned. - * \return A vector of child namespace names. + * \param[in] parent_namespace_ident The optional parent namespace. + * \return A vector of child namespaces. */ - Result> ListChildrenNamespaces( - const std::optional& parent_namespace_ident = std::nullopt) const; + Result> ListNamespaces( + const Namespace& parent_namespace_ident) const; /** * \brief Creates a new namespace with the specified properties. @@ -72,7 +71,7 @@ class ICEBERG_EXPORT InMemoryNamespace { * \param[in] namespace_ident The namespace to delete. * \return Status indicating success or failure. */ - Status DeleteNamespace(const Namespace& namespace_ident); + Status DropNamespace(const Namespace& namespace_ident); /** * \brief Retrieves the properties of the specified namespace. @@ -89,10 +88,20 @@ class ICEBERG_EXPORT InMemoryNamespace { * \param[in] properties The new properties map. * \return Status indicating success or failure. */ - Status ReplaceProperties( + Status SetNamespaceProperties( const Namespace& namespace_ident, const std::unordered_map& properties); + /// \brief Remove a set of metadata properties from a namespace. + /// + /// \param namespace_ident the namespace to modify + /// \param properties the set of property keys to remove + /// \return Status::OK if removed successfully; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist; + /// ErrorKind::kNotSupported if the operation is not supported + Status RemoveNamespaceProperties(const Namespace& namespace_ident, + const std::unordered_set& properties); + /** * \brief Lists all table names under the specified namespace. * \param[in] namespace_ident The namespace from which to list tables. @@ -166,26 +175,31 @@ Result GetNamespace(const InMemoryNamespace* root, return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident); } -Status InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const { - const auto ns = GetNamespace(this, namespace_ident); - ICEBERG_RETURN_UNEXPECTED(ns); - return {}; +Result InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const { + const auto& ns = GetNamespace(this, namespace_ident); + if (ns.has_value()) { + return true; + } + if (ns.error().kind == ErrorKind::kNoSuchNamespace) { + return false; + } + return unexpected(ns.error()); } -Result> InMemoryNamespace::ListChildrenNamespaces( - const std::optional& parent_namespace_ident) const { - auto ns = this; - if (parent_namespace_ident.has_value()) { - const auto nsRs = GetNamespace(this, *parent_namespace_ident); - ICEBERG_RETURN_UNEXPECTED(nsRs); - ns = *nsRs; - } +Result> InMemoryNamespace::ListNamespaces( + const Namespace& parent_namespace_ident) const { + const auto nsRs = GetNamespace(this, parent_namespace_ident); + ICEBERG_RETURN_UNEXPECTED(nsRs); + auto ns = *nsRs; - std::vector names; + std::vector names; auto const& children = ns->children_; names.reserve(children.size()); - std::ranges::transform(children, std::back_inserter(names), - [](const auto& pair) { return pair.first; }); + std::ranges::transform(children, std::back_inserter(names), [&](const auto& pair) { + auto childNs = parent_namespace_ident; + childNs.levels.emplace_back(pair.first); + return childNs; + }); return names; } @@ -214,7 +228,7 @@ Status InMemoryNamespace::CreateNamespace( return {}; } -Status InMemoryNamespace::DeleteNamespace(const Namespace& namespace_ident) { +Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) { if (namespace_ident.levels.empty()) { return InvalidArgument("namespace identifier is empty"); } @@ -247,7 +261,7 @@ Result> InMemoryNamespace::GetPrope return ns.value()->properties_; } -Status InMemoryNamespace::ReplaceProperties( +Status InMemoryNamespace::SetNamespaceProperties( const Namespace& namespace_ident, const std::unordered_map& properties) { const auto ns = GetNamespace(this, namespace_ident); @@ -256,6 +270,15 @@ Status InMemoryNamespace::ReplaceProperties( return {}; } +Status InMemoryNamespace::RemoveNamespaceProperties( + const Namespace& namespace_ident, const std::unordered_set& properties) { + const auto ns = GetNamespace(this, namespace_ident); + ICEBERG_RETURN_UNEXPECTED(ns); + std::ranges::for_each(properties, + [&](const auto& prop) { ns.value()->properties_.erase(prop); }); + return {}; +} + Result> InMemoryNamespace::ListTables( const Namespace& namespace_ident) const { const auto ns = GetNamespace(this, namespace_ident); @@ -317,6 +340,25 @@ class ICEBERG_EXPORT InMemoryCatalogImpl { std::string_view name() const; + Status CreateNamespace(const Namespace& ns, + const std::unordered_map& properties); + + Result> ListNamespaces(const Namespace& ns) const; + + Status DropNamespace(const Namespace& ns); + + Result NamespaceExists(const Namespace& ns) const; + + Result> GetNamespaceProperties( + const Namespace& ns) const; + + Status SetNamespaceProperties( + const Namespace& ns, + const std::unordered_map& properties); + + Status RemoveNamespaceProperties(const Namespace& ns, + const std::unordered_set& properties); + Result> ListTables(const Namespace& ns) const; Result> CreateTable( @@ -366,6 +408,46 @@ InMemoryCatalogImpl::InMemoryCatalogImpl( std::string_view InMemoryCatalogImpl::name() const { return catalog_name_; } +Status InMemoryCatalogImpl::CreateNamespace( + const Namespace& ns, const std::unordered_map& properties) { + std::unique_lock lock(mutex_); + return root_namespace_->CreateNamespace(ns, properties); +} + +Result> InMemoryCatalogImpl::ListNamespaces( + const Namespace& ns) const { + std::unique_lock lock(mutex_); + return root_namespace_->ListNamespaces(ns); +} + +Status InMemoryCatalogImpl::DropNamespace(const Namespace& ns) { + std::unique_lock lock(mutex_); + return root_namespace_->DropNamespace(ns); +} + +Result InMemoryCatalogImpl::NamespaceExists(const Namespace& ns) const { + std::unique_lock lock(mutex_); + return root_namespace_->NamespaceExists(ns); +} + +Result> +InMemoryCatalogImpl::GetNamespaceProperties(const Namespace& ns) const { + std::unique_lock lock(mutex_); + return root_namespace_->GetProperties(ns); +} + +Status InMemoryCatalogImpl::SetNamespaceProperties( + const Namespace& ns, const std::unordered_map& properties) { + std::unique_lock lock(mutex_); + return root_namespace_->SetNamespaceProperties(ns, properties); +} + +Status InMemoryCatalogImpl::RemoveNamespaceProperties( + const Namespace& ns, const std::unordered_set& properties) { + std::unique_lock lock(mutex_); + return root_namespace_->RemoveNamespaceProperties(ns, properties); +} + Result> InMemoryCatalogImpl::ListTables( const Namespace& ns) const { std::unique_lock lock(mutex_); @@ -444,6 +526,39 @@ InMemoryCatalog::~InMemoryCatalog() = default; std::string_view InMemoryCatalog::name() const { return impl_->name(); } +Status InMemoryCatalog::CreateNamespace( + const Namespace& ns, const std::unordered_map& properties) { + return impl_->CreateNamespace(ns, properties); +} + +Result> +InMemoryCatalog::GetNamespaceProperties(const Namespace& ns) const { + return impl_->GetNamespaceProperties(ns); +} + +Result> InMemoryCatalog::ListNamespaces( + const Namespace& ns) const { + return impl_->ListNamespaces(ns); +} + +Status InMemoryCatalog::DropNamespace(const Namespace& ns) { + return impl_->DropNamespace(ns); +} + +Result InMemoryCatalog::NamespaceExists(const Namespace& ns) const { + return impl_->NamespaceExists(ns); +} + +Status InMemoryCatalog::RemoveNamespaceProperties( + const Namespace& ns, const std::unordered_set& properties) { + return impl_->RemoveNamespaceProperties(ns, properties); +} + +Status InMemoryCatalog::SetNamespaceProperties( + const Namespace& ns, const std::unordered_map& properties) { + return impl_->SetNamespaceProperties(ns, properties); +} + Result> InMemoryCatalog::ListTables( const Namespace& ns) const { return impl_->ListTables(ns); diff --git a/src/iceberg/catalog/in_memory_catalog.h b/src/iceberg/catalog/in_memory_catalog.h index 6063852a8..e3121a3c4 100644 --- a/src/iceberg/catalog/in_memory_catalog.h +++ b/src/iceberg/catalog/in_memory_catalog.h @@ -22,7 +22,16 @@ #include "iceberg/catalog.h" namespace iceberg { - +/** + * @brief An in-memory implementation of the Iceberg Catalog interface. + * + * This catalog stores all metadata purely in memory, with no persistence to disk + * or external systems. It is primarily intended for unit tests, prototyping, or + * demonstration purposes. + * + * @note This class is **not** suitable for production use. + * All data will be lost when the process exits. + */ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { public: InMemoryCatalog(std::string const& name, std::shared_ptr const& file_io, @@ -32,6 +41,26 @@ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { std::string_view name() const override; + Status CreateNamespace( + const Namespace& ns, + const std::unordered_map& properties) override; + + Result> ListNamespaces(const Namespace& ns) const override; + + Status DropNamespace(const Namespace& ns) override; + + Result NamespaceExists(const Namespace& ns) const override; + + Result> GetNamespaceProperties( + const Namespace& ns) const override; + + Status SetNamespaceProperties( + const Namespace& ns, + const std::unordered_map& properties) override; + + Status RemoveNamespaceProperties( + const Namespace& ns, const std::unordered_set& properties) override; + Result> ListTables(const Namespace& ns) const override; Result> CreateTable( diff --git a/test/in_memory_catalog_test.cc b/test/in_memory_catalog_test.cc index 60a3b0024..70d9a909f 100644 --- a/test/in_memory_catalog_test.cc +++ b/test/in_memory_catalog_test.cc @@ -64,4 +64,29 @@ TEST_F(InMemoryCatalogTest, DropTable) { EXPECT_THAT(result, IsOk()); } +TEST_F(InMemoryCatalogTest, Namespace) { + Namespace ns{.levels = {"n1", "n2"}}; + std::unordered_map properties = {{"prop1", "val1"}}; + EXPECT_THAT(catalog_->CreateNamespace(ns, properties), IsOk()); + EXPECT_THAT(catalog_->CreateNamespace(ns, properties), + IsError(ErrorKind::kAlreadyExists)); + + EXPECT_THAT(catalog_->NamespaceExists(ns), HasValue(::testing::Eq(true))); + EXPECT_THAT(catalog_->NamespaceExists(Namespace{.levels = {"n1", "n3"}}), + HasValue(::testing::Eq(false))); + auto childNs = catalog_->ListNamespaces(Namespace{.levels = {"n1"}}); + EXPECT_THAT(childNs, IsOk()); + ASSERT_EQ(childNs->size(), 1U); + ASSERT_EQ(childNs->at(0).levels.size(), 2U); + ASSERT_EQ(childNs->at(0).levels.at(1), "n2"); + + auto propsRs = catalog_->GetNamespaceProperties(ns); + EXPECT_THAT(propsRs, IsOk()); + ASSERT_EQ(propsRs->size(), 1U); + ASSERT_EQ(propsRs.value().at("prop1"), "val1"); + + EXPECT_THAT(catalog_->SetNamespaceProperties(ns, {{"prop2", "val2"}}), IsOk()); + EXPECT_THAT(catalog_->RemoveNamespaceProperties(ns, {"prop2"}), IsOk()); +} + } // namespace iceberg From 99675e4a5a7307e1d6c944de4f15cf264e3c0cfe Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Mon, 26 May 2025 12:12:04 +0800 Subject: [PATCH 9/9] Change "set/remove namespace properties" to "update namespace properties" --- src/iceberg/catalog.h | 27 +-- src/iceberg/catalog/in_memory_catalog.cc | 210 ++++++++++------------- src/iceberg/catalog/in_memory_catalog.h | 9 +- test/in_memory_catalog_test.cc | 16 +- 4 files changed, 117 insertions(+), 145 deletions(-) diff --git a/src/iceberg/catalog.h b/src/iceberg/catalog.h index b7a632bc6..a882f4d61 100644 --- a/src/iceberg/catalog.h +++ b/src/iceberg/catalog.h @@ -83,26 +83,17 @@ class ICEBERG_EXPORT Catalog { /// \return true if the namespace exists, false otherwise virtual Result NamespaceExists(const Namespace& ns) const = 0; - /// \brief Set metadata properties on a namespace. + /// \brief Update a namespace's properties by applying additions and removals. /// - /// \param ns the namespace to modify - /// \param properties the properties to set or update - /// \return Status::OK if updated successfully; + /// \param ns the namespace to update + /// \param updates a set of properties to add or overwrite + /// \param removals a set of property keys to remove + /// \return Status::OK if the update is successful; /// ErrorKind::kNoSuchNamespace if the namespace does not exist; - /// ErrorKind::kNotSupported if the operation is not supported - virtual Status SetNamespaceProperties( - const Namespace& ns, - const std::unordered_map& properties) = 0; - - /// \brief Remove a set of metadata properties from a namespace. - /// - /// \param ns the namespace to modify - /// \param properties the set of property keys to remove - /// \return Status::OK if removed successfully; - /// ErrorKind::kNoSuchNamespace if the namespace does not exist; - /// ErrorKind::kNotSupported if the operation is not supported - virtual Status RemoveNamespaceProperties( - const Namespace& ns, const std::unordered_set& properties) = 0; + /// ErrorKind::kUnsupported if the operation is not supported + virtual Status UpdateNamespaceProperties( + const Namespace& ns, const std::unordered_map& updates, + const std::unordered_set& removals) = 0; /// \brief Return all the identifiers under this namespace /// diff --git a/src/iceberg/catalog/in_memory_catalog.cc b/src/iceberg/catalog/in_memory_catalog.cc index 7175be4ca..3e32ddc75 100644 --- a/src/iceberg/catalog/in_memory_catalog.cc +++ b/src/iceberg/catalog/in_memory_catalog.cc @@ -33,113 +33,104 @@ namespace iceberg { namespace { -/** - * \brief A hierarchical namespace that manages namespaces and table metadata in-memory. - * - * Each InMemoryNamespace represents a namespace level and can contain properties, - * tables, and child namespaces. This structure enables a tree-like representation - * of nested namespaces. - */ +/// \brief A hierarchical namespace that manages namespaces and table metadata in-memory. +/// +/// Each InMemoryNamespace represents a namespace level and can contain properties, +/// tables, and child namespaces. This structure enables a tree-like representation +/// of nested namespaces. class ICEBERG_EXPORT InMemoryNamespace { public: - /** - * \brief Checks whether the given namespace exists. - * \param[in] namespace_ident The namespace to check. - * \return Status indicating success or failure. - */ + /// \brief Checks whether the given namespace exists. + /// + /// \param namespace_ident The namespace to check. + /// \return Result indicating whether the namespace exists. Result NamespaceExists(const Namespace& namespace_ident) const; - /** - * \brief Lists immediate child namespaces under the given parent namespace. - * \param[in] parent_namespace_ident The optional parent namespace. - * \return A vector of child namespaces. - */ + /// \brief Lists immediate child namespaces under the given parent namespace. + /// + /// \param parent_namespace_ident The parent namespace to list children for. + /// \return A vector of child namespaces if found; error otherwise. Result> ListNamespaces( const Namespace& parent_namespace_ident) const; - /** - * \brief Creates a new namespace with the specified properties. - * \param[in] namespace_ident The namespace to create. - * \param[in] properties A map of key-value pairs to associate with the namespace. - * \return Status indicating success or failure. - */ + /// \brief Creates a new namespace with the specified properties. + /// + /// \param namespace_ident The namespace to create. + /// \param properties A map of key-value pairs to associate with the namespace. + /// \return Status::OK if the namespace is created; + /// ErrorKind::kAlreadyExists if the namespace already exists. Status CreateNamespace(const Namespace& namespace_ident, const std::unordered_map& properties); - /** - * \brief Deletes an existing namespace. - * \param[in] namespace_ident The namespace to delete. - * \return Status indicating success or failure. - */ + /// \brief Deletes an existing namespace. + /// + /// \param namespace_ident The namespace to delete. + /// \return Status::OK if the namespace is deleted; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist; + /// ErrorKind::kNotAllowed if the namespace is not empty. Status DropNamespace(const Namespace& namespace_ident); - /** - * \brief Retrieves the properties of the specified namespace. - * \param[in] namespace_ident The namespace whose properties to retrieve. - * \return An containing the properties map if the namespace exists; - * Errpr otherwise. - */ + /// \brief Retrieves the properties of the specified namespace. + /// + /// \param namespace_ident The namespace whose properties are to be retrieved. + /// \return A map of property key-value pairs if the namespace exists; + /// error otherwise. Result> GetProperties( const Namespace& namespace_ident) const; - /** - * \brief Replaces all properties of the given namespace. - * \param[in] namespace_ident The namespace whose properties will be replaced. - * \param[in] properties The new properties map. - * \return Status indicating success or failure. - */ - Status SetNamespaceProperties( + /// \brief Updates a namespace's properties by applying additions and removals. + /// + /// \param namespace_ident The namespace to update. + /// \param updates Properties to add or overwrite. + /// \param removals Property keys to remove. + /// \return Status::OK if the update is successful; + /// ErrorKind::kNoSuchNamespace if the namespace does not exist; + /// ErrorKind::kUnsupported if the operation is not supported. + Status UpdateNamespaceProperties( const Namespace& namespace_ident, - const std::unordered_map& properties); + const std::unordered_map& updates, + const std::unordered_set& removals); - /// \brief Remove a set of metadata properties from a namespace. + /// \brief Lists all table names under the specified namespace. /// - /// \param namespace_ident the namespace to modify - /// \param properties the set of property keys to remove - /// \return Status::OK if removed successfully; - /// ErrorKind::kNoSuchNamespace if the namespace does not exist; - /// ErrorKind::kNotSupported if the operation is not supported - Status RemoveNamespaceProperties(const Namespace& namespace_ident, - const std::unordered_set& properties); - - /** - * \brief Lists all table names under the specified namespace. - * \param[in] namespace_ident The namespace from which to list tables. - * \return A vector of table names or error. - */ + /// \param namespace_ident The namespace to list tables from. + /// \return A vector of table names if successful; error otherwise. Result> ListTables(const Namespace& namespace_ident) const; - /** - * \brief Registers a table in the given namespace with a metadata location. - * \param[in] table_ident The fully qualified identifier of the table. - * \param[in] metadata_location The path to the table's metadata. - * \return Status indicating success or failure. - */ - Status RegisterTable(TableIdentifier const& table_ident, + /// \brief Registers a table in the given namespace with a metadata location. + /// + /// \param table_ident The fully qualified identifier of the table. + /// \param metadata_location The path to the table's metadata. + /// \return Status::OK if the table is registered; + /// Error otherwise. + Status RegisterTable(const TableIdentifier& table_ident, const std::string& metadata_location); - /** - * \brief Unregisters a table from the specified namespace. - * \param[in] table_ident The identifier of the table to unregister. - * \return Status indicating success or failure. - */ - Status UnregisterTable(TableIdentifier const& table_ident); - - /** - * \brief Checks if a table exists in the specified namespace. - * \param[in] table_ident The identifier of the table to check. - * \return Result indicating table exists or not. - */ - Result TableExists(TableIdentifier const& table_ident) const; - - /** - * \brief Gets the metadata location for the specified table. - * \param[in] table_ident The identifier of the table. - * \return An string containing the metadata location if the table exists; - * Error otherwise. - */ - Result GetTableMetadataLocation(TableIdentifier const& table_ident) const; + /// \brief Unregisters a table from the specified namespace. + /// + /// \param table_ident The identifier of the table to unregister. + /// \return Status::OK if the table is removed; + /// ErrorKind::kNoSuchTable if the table does not exist. + Status UnregisterTable(const TableIdentifier& table_ident); + + /// \brief Checks if a table exists in the specified namespace. + /// + /// \param table_ident The identifier of the table to check. + /// \return Result indicating whether the table exists. + Result TableExists(const TableIdentifier& table_ident) const; + + /// \brief Gets the metadata location for the specified table. + /// + /// \param table_ident The identifier of the table. + /// \return The metadata location if the table exists; error otherwise. + Result GetTableMetadataLocation(const TableIdentifier& table_ident) const; + /// \brief Internal utility for retrieving a namespace node pointer from the tree. + /// + /// \tparam NamespacePtr The type of the namespace node pointer. + /// \param root The root namespace node. + /// \param namespace_ident The fully qualified namespace to resolve. + /// \return A pointer to the namespace node if it exists; error otherwise. template static Result GetNamespaceImpl(NamespacePtr root, const Namespace& namespace_ident) { @@ -261,20 +252,17 @@ Result> InMemoryNamespace::GetPrope return ns.value()->properties_; } -Status InMemoryNamespace::SetNamespaceProperties( +Status InMemoryNamespace::UpdateNamespaceProperties( const Namespace& namespace_ident, - const std::unordered_map& properties) { + const std::unordered_map& updates, + const std::unordered_set& removals) { const auto ns = GetNamespace(this, namespace_ident); ICEBERG_RETURN_UNEXPECTED(ns); - ns.value()->properties_ = properties; - return {}; -} -Status InMemoryNamespace::RemoveNamespaceProperties( - const Namespace& namespace_ident, const std::unordered_set& properties) { - const auto ns = GetNamespace(this, namespace_ident); - ICEBERG_RETURN_UNEXPECTED(ns); - std::ranges::for_each(properties, + std::ranges::for_each(updates, [&](const auto& prop) { + ns.value()->properties_[prop.first] = prop.second; + }); + std::ranges::for_each(removals, [&](const auto& prop) { ns.value()->properties_.erase(prop); }); return {}; } @@ -352,12 +340,9 @@ class ICEBERG_EXPORT InMemoryCatalogImpl { Result> GetNamespaceProperties( const Namespace& ns) const; - Status SetNamespaceProperties( - const Namespace& ns, - const std::unordered_map& properties); - - Status RemoveNamespaceProperties(const Namespace& ns, - const std::unordered_set& properties); + Status UpdateNamespaceProperties( + const Namespace& ns, const std::unordered_map& updates, + const std::unordered_set& removals); Result> ListTables(const Namespace& ns) const; @@ -436,16 +421,11 @@ InMemoryCatalogImpl::GetNamespaceProperties(const Namespace& ns) const { return root_namespace_->GetProperties(ns); } -Status InMemoryCatalogImpl::SetNamespaceProperties( - const Namespace& ns, const std::unordered_map& properties) { - std::unique_lock lock(mutex_); - return root_namespace_->SetNamespaceProperties(ns, properties); -} - -Status InMemoryCatalogImpl::RemoveNamespaceProperties( - const Namespace& ns, const std::unordered_set& properties) { +Status InMemoryCatalogImpl::UpdateNamespaceProperties( + const Namespace& ns, const std::unordered_map& updates, + const std::unordered_set& removals) { std::unique_lock lock(mutex_); - return root_namespace_->RemoveNamespaceProperties(ns, properties); + return root_namespace_->UpdateNamespaceProperties(ns, updates, removals); } Result> InMemoryCatalogImpl::ListTables( @@ -549,14 +529,10 @@ Result InMemoryCatalog::NamespaceExists(const Namespace& ns) const { return impl_->NamespaceExists(ns); } -Status InMemoryCatalog::RemoveNamespaceProperties( - const Namespace& ns, const std::unordered_set& properties) { - return impl_->RemoveNamespaceProperties(ns, properties); -} - -Status InMemoryCatalog::SetNamespaceProperties( - const Namespace& ns, const std::unordered_map& properties) { - return impl_->SetNamespaceProperties(ns, properties); +Status InMemoryCatalog::UpdateNamespaceProperties( + const Namespace& ns, const std::unordered_map& updates, + const std::unordered_set& removals) { + return impl_->UpdateNamespaceProperties(ns, updates, removals); } Result> InMemoryCatalog::ListTables( diff --git a/src/iceberg/catalog/in_memory_catalog.h b/src/iceberg/catalog/in_memory_catalog.h index e3121a3c4..c8e24b5db 100644 --- a/src/iceberg/catalog/in_memory_catalog.h +++ b/src/iceberg/catalog/in_memory_catalog.h @@ -54,12 +54,9 @@ class ICEBERG_EXPORT InMemoryCatalog : public Catalog { Result> GetNamespaceProperties( const Namespace& ns) const override; - Status SetNamespaceProperties( - const Namespace& ns, - const std::unordered_map& properties) override; - - Status RemoveNamespaceProperties( - const Namespace& ns, const std::unordered_set& properties) override; + Status UpdateNamespaceProperties( + const Namespace& ns, const std::unordered_map& updates, + const std::unordered_set& removals) override; Result> ListTables(const Namespace& ns) const override; diff --git a/test/in_memory_catalog_test.cc b/test/in_memory_catalog_test.cc index 70d9a909f..c76d78878 100644 --- a/test/in_memory_catalog_test.cc +++ b/test/in_memory_catalog_test.cc @@ -66,7 +66,8 @@ TEST_F(InMemoryCatalogTest, DropTable) { TEST_F(InMemoryCatalogTest, Namespace) { Namespace ns{.levels = {"n1", "n2"}}; - std::unordered_map properties = {{"prop1", "val1"}}; + std::unordered_map properties = {{"prop1", "val1"}, + {"prop2", "val2"}}; EXPECT_THAT(catalog_->CreateNamespace(ns, properties), IsOk()); EXPECT_THAT(catalog_->CreateNamespace(ns, properties), IsError(ErrorKind::kAlreadyExists)); @@ -82,11 +83,18 @@ TEST_F(InMemoryCatalogTest, Namespace) { auto propsRs = catalog_->GetNamespaceProperties(ns); EXPECT_THAT(propsRs, IsOk()); - ASSERT_EQ(propsRs->size(), 1U); + ASSERT_EQ(propsRs->size(), 2U); ASSERT_EQ(propsRs.value().at("prop1"), "val1"); + ASSERT_EQ(propsRs.value().at("prop2"), "val2"); - EXPECT_THAT(catalog_->SetNamespaceProperties(ns, {{"prop2", "val2"}}), IsOk()); - EXPECT_THAT(catalog_->RemoveNamespaceProperties(ns, {"prop2"}), IsOk()); + EXPECT_THAT(catalog_->UpdateNamespaceProperties( + ns, {{"prop2", "val2-updated"}, {"prop3", "val3"}}, {"prop1"}), + IsOk()); + propsRs = catalog_->GetNamespaceProperties(ns); + EXPECT_THAT(propsRs, IsOk()); + ASSERT_EQ(propsRs->size(), 2U); + ASSERT_EQ(propsRs.value().at("prop2"), "val2-updated"); + ASSERT_EQ(propsRs.value().at("prop3"), "val3"); } } // namespace iceberg