From c0fdd0d87c27bb6d1a03f8d33bdb29fbc7a7ce07 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Thu, 25 Dec 2025 11:28:09 +0800 Subject: [PATCH 1/4] feat(rest):implement load table --- src/iceberg/catalog/rest/rest_catalog.cc | 17 ++- src/iceberg/test/rest_catalog_test.cc | 171 ++++++++++++++--------- 2 files changed, 117 insertions(+), 71 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index eeffffd26..fd8bea0bf 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -309,9 +309,20 @@ Status RestCatalog::RenameTable([[maybe_unused]] const TableIdentifier& from, return NotImplemented("Not implemented"); } -Result> RestCatalog::LoadTable( - [[maybe_unused]] const TableIdentifier& identifier) { - return NotImplemented("Not implemented"); +Result> RestCatalog::LoadTable(const TableIdentifier& identifier) { + ICEBERG_RETURN_UNEXPECTED(CheckEndpoint(supported_endpoints_, Endpoint::LoadTable())); + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier)); + + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + client_->Get(path, /*params=*/{}, /*headers=*/{}, *TableErrorHandler::Instance())); + + // TODO(Feiyang Li): support load metadata table + ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); + ICEBERG_ASSIGN_OR_RAISE(auto load_result, LoadTableResultFromJson(json)); + return Table::Make(identifier, load_result.metadata, + std::move(load_result.metadata_location), file_io_, + shared_from_this()); } Result> RestCatalog::RegisterTable( diff --git a/src/iceberg/test/rest_catalog_test.cc b/src/iceberg/test/rest_catalog_test.cc index fb1f70610..0ff36807b 100644 --- a/src/iceberg/test/rest_catalog_test.cc +++ b/src/iceberg/test/rest_catalog_test.cc @@ -136,6 +136,45 @@ class RestCatalogIntegrationTest : public ::testing::Test { return RestCatalog::Make(*config, std::make_shared()); } + // Helper function to create and verify a catalog (asserts on failure) + std::shared_ptr CreateAndVerifyCatalog() { + auto catalog_result = CreateCatalog(); + EXPECT_THAT(catalog_result, IsOk()); + return catalog_result.value(); + } + + // Helper function to create and verify a namespace + void CreateAndVerifyNamespace( + RestCatalog& catalog, const Namespace& ns, + const std::unordered_map& properties = {}) { + auto status = catalog.CreateNamespace(ns, properties); + EXPECT_THAT(status, IsOk()); + } + + // Helper function to create a default schema for testing + std::shared_ptr CreateDefaultSchema() { + return std::make_shared( + std::vector{SchemaField::MakeRequired(1, "id", int32()), + SchemaField::MakeOptional(2, "data", string())}, + /*schema_id=*/1); + } + + // Helper function to create a default unpartitioned partition spec + std::shared_ptr CreateDefaultPartitionSpec() { + auto partition_spec_result = + PartitionSpec::Make(PartitionSpec::kInitialSpecId, {}, 0); + EXPECT_THAT(partition_spec_result, IsOk()); + return std::shared_ptr(std::move(*partition_spec_result)); + } + + // Helper function to create a default unsorted sort order + std::shared_ptr CreateDefaultSortOrder() { + auto sort_order_result = + SortOrder::Make(SortOrder::kUnsortedOrderId, std::vector{}); + EXPECT_THAT(sort_order_result, IsOk()); + return std::shared_ptr(std::move(*sort_order_result)); + } + static inline std::unique_ptr docker_compose_; }; @@ -182,9 +221,7 @@ TEST_F(RestCatalogIntegrationTest, FetchServerConfigDirect) { } TEST_F(RestCatalogIntegrationTest, ListNamespaces) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); Namespace root{.levels = {}}; auto result = catalog->ListNamespaces(root); @@ -193,14 +230,11 @@ TEST_F(RestCatalogIntegrationTest, ListNamespaces) { } TEST_F(RestCatalogIntegrationTest, CreateNamespace) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create a simple namespace Namespace ns{.levels = {"test_ns"}}; - auto status = catalog->CreateNamespace(ns, {}); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, ns); // Verify it was created by listing Namespace root{.levels = {}}; @@ -211,16 +245,13 @@ TEST_F(RestCatalogIntegrationTest, CreateNamespace) { } TEST_F(RestCatalogIntegrationTest, CreateNamespaceWithProperties) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create namespace with properties Namespace ns{.levels = {"test_ns_props"}}; std::unordered_map properties{ {"owner", "test_user"}, {"description", "Test namespace with properties"}}; - auto status = catalog->CreateNamespace(ns, properties); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, ns, properties); // Verify properties were set auto props_result = catalog->GetNamespaceProperties(ns); @@ -230,19 +261,15 @@ TEST_F(RestCatalogIntegrationTest, CreateNamespaceWithProperties) { } TEST_F(RestCatalogIntegrationTest, CreateNestedNamespace) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create parent namespace Namespace parent{.levels = {"parent"}}; - auto status = catalog->CreateNamespace(parent, {}); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, parent); // Create nested namespace Namespace child{.levels = {"parent", "child"}}; - status = catalog->CreateNamespace(child, {}); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, child); // Verify nested namespace exists auto list_result = catalog->ListNamespaces(parent); @@ -252,16 +279,13 @@ TEST_F(RestCatalogIntegrationTest, CreateNestedNamespace) { } TEST_F(RestCatalogIntegrationTest, GetNamespaceProperties) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create namespace with properties Namespace ns{.levels = {"test_get_props"}}; std::unordered_map properties{{"key1", "value1"}, {"key2", "value2"}}; - auto status = catalog->CreateNamespace(ns, properties); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, ns, properties); // Get properties auto props_result = catalog->GetNamespaceProperties(ns); @@ -271,9 +295,7 @@ TEST_F(RestCatalogIntegrationTest, GetNamespaceProperties) { } TEST_F(RestCatalogIntegrationTest, NamespaceExists) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Check non-existent namespace Namespace ns{.levels = {"non_existent"}}; @@ -282,8 +304,7 @@ TEST_F(RestCatalogIntegrationTest, NamespaceExists) { EXPECT_FALSE(*exists_result); // Create namespace - auto status = catalog->CreateNamespace(ns, {}); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, ns); // Check it now exists exists_result = catalog->NamespaceExists(ns); @@ -292,22 +313,19 @@ TEST_F(RestCatalogIntegrationTest, NamespaceExists) { } TEST_F(RestCatalogIntegrationTest, UpdateNamespaceProperties) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create namespace with initial properties Namespace ns{.levels = {"test_update"}}; std::unordered_map initial_props{{"key1", "value1"}, {"key2", "value2"}}; - auto status = catalog->CreateNamespace(ns, initial_props); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, ns, initial_props); // Update properties: modify key1, add key3, remove key2 std::unordered_map updates{{"key1", "updated_value1"}, {"key3", "value3"}}; std::unordered_set removals{"key2"}; - status = catalog->UpdateNamespaceProperties(ns, updates, removals); + auto status = catalog->UpdateNamespaceProperties(ns, updates, removals); EXPECT_THAT(status, IsOk()); // Verify updated properties @@ -319,14 +337,11 @@ TEST_F(RestCatalogIntegrationTest, UpdateNamespaceProperties) { } TEST_F(RestCatalogIntegrationTest, DropNamespace) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create namespace Namespace ns{.levels = {"test_drop"}}; - auto status = catalog->CreateNamespace(ns, {}); - EXPECT_THAT(status, IsOk()); + CreateAndVerifyNamespace(*catalog, ns); // Verify it exists auto exists_result = catalog->NamespaceExists(ns); @@ -334,7 +349,7 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) { EXPECT_TRUE(*exists_result); // Drop namespace - status = catalog->DropNamespace(ns); + auto status = catalog->DropNamespace(ns); EXPECT_THAT(status, IsOk()); // Verify it no longer exists @@ -344,9 +359,7 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) { } TEST_F(RestCatalogIntegrationTest, CreateTable) { - auto catalog_result = CreateCatalog(); - ASSERT_THAT(catalog_result, IsOk()); - auto& catalog = catalog_result.value(); + auto catalog = CreateAndVerifyCatalog(); // Create nested namespace with properties Namespace ns{.levels = {"test_create_table", "apple", "ios"}}; @@ -354,30 +367,13 @@ TEST_F(RestCatalogIntegrationTest, CreateTable) { {"community", "apache"}}; // Create parent namespaces first - auto status = catalog->CreateNamespace(Namespace{.levels = {"test_create_table"}}, {}); - EXPECT_THAT(status, IsOk()); - status = - catalog->CreateNamespace(Namespace{.levels = {"test_create_table", "apple"}}, {}); - EXPECT_THAT(status, IsOk()); - status = catalog->CreateNamespace(ns, ns_properties); - EXPECT_THAT(status, IsOk()); - - // Create schema - auto schema = std::make_shared( - std::vector{SchemaField::MakeOptional(1, "foo", string()), - SchemaField::MakeRequired(2, "bar", int32()), - SchemaField::MakeOptional(3, "baz", boolean())}, - /*schema_id=*/1); + CreateAndVerifyNamespace(*catalog, Namespace{.levels = {"test_create_table"}}); + CreateAndVerifyNamespace(*catalog, Namespace{.levels = {"test_create_table", "apple"}}); + CreateAndVerifyNamespace(*catalog, ns, ns_properties); - // Create partition spec and sort order (unpartitioned and unsorted) - auto partition_spec_result = PartitionSpec::Make(PartitionSpec::kInitialSpecId, {}, 0); - ASSERT_THAT(partition_spec_result, IsOk()); - auto partition_spec = std::shared_ptr(std::move(*partition_spec_result)); - - auto sort_order_result = - SortOrder::Make(SortOrder::kUnsortedOrderId, std::vector{}); - ASSERT_THAT(sort_order_result, IsOk()); - auto sort_order = std::shared_ptr(std::move(*sort_order_result)); + auto schema = CreateDefaultSchema(); + auto partition_spec = CreateDefaultPartitionSpec(); + auto sort_order = CreateDefaultSortOrder(); // Create table TableIdentifier table_id{.ns = ns, .name = "t1"}; @@ -400,4 +396,43 @@ TEST_F(RestCatalogIntegrationTest, CreateTable) { HasErrorMessage("Table already exists: test_create_table.apple.ios.t1")); } +TEST_F(RestCatalogIntegrationTest, LoadTable) { + auto catalog = CreateAndVerifyCatalog(); + + // Create namespace and table first + Namespace ns{.levels = {"test_load_table"}}; + CreateAndVerifyNamespace(*catalog, ns); + + // Create schema, partition spec, and sort order using helper functions + auto schema = CreateDefaultSchema(); + auto partition_spec = CreateDefaultPartitionSpec(); + auto sort_order = CreateDefaultSortOrder(); + + // Create table + TableIdentifier table_id{.ns = ns, .name = "test_table"}; + std::unordered_map table_properties{{"key1", "value1"}}; + auto create_result = catalog->CreateTable(table_id, schema, partition_spec, sort_order, + "", table_properties); + ASSERT_THAT(create_result, IsOk()); + + // Load the table + auto load_result = catalog->LoadTable(table_id); + ASSERT_THAT(load_result, IsOk()); + auto& loaded_table = load_result.value(); + + // Verify loaded table properties + EXPECT_EQ(loaded_table->name().ns.levels, std::vector{"test_load_table"}); + EXPECT_EQ(loaded_table->name().name, "test_table"); + EXPECT_NE(loaded_table->metadata(), nullptr); + + // Verify schema + auto loaded_schema_result = loaded_table->schema(); + ASSERT_THAT(loaded_schema_result, IsOk()); + auto loaded_schema = loaded_schema_result.value(); + EXPECT_TRUE(loaded_schema->schema_id().has_value()); // Server assigns schema_id + EXPECT_EQ(loaded_schema->fields().size(), 2); + EXPECT_EQ(loaded_schema->fields()[0].name(), "id"); + EXPECT_EQ(loaded_schema->fields()[1].name(), "data"); +} + } // namespace iceberg::rest From 10c4e025c35319fa1d0663e91adaac50b3798472 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Thu, 25 Dec 2025 14:50:55 +0800 Subject: [PATCH 2/4] feat(rest):implement drop table --- src/iceberg/catalog.h | 6 +- .../catalog/memory/in_memory_catalog.cc | 36 +++++---- .../catalog/memory/in_memory_catalog.h | 4 +- src/iceberg/catalog/rest/http_client.cc | 5 +- src/iceberg/catalog/rest/http_client.h | 1 + src/iceberg/catalog/rest/rest_catalog.cc | 42 +++++++--- src/iceberg/catalog/rest/rest_catalog.h | 4 +- src/iceberg/test/mock_catalog.h | 4 +- src/iceberg/test/rest_catalog_test.cc | 76 +++++++++++++++++++ 9 files changed, 141 insertions(+), 37 deletions(-) diff --git a/src/iceberg/catalog.h b/src/iceberg/catalog.h index 7be56bae1..976defe95 100644 --- a/src/iceberg/catalog.h +++ b/src/iceberg/catalog.h @@ -75,7 +75,7 @@ class ICEBERG_EXPORT Catalog { /// \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; + virtual Result DropNamespace(const Namespace& ns) = 0; /// \brief Check whether the namespace exists. /// @@ -160,10 +160,10 @@ class ICEBERG_EXPORT Catalog { /// /// \param identifier a table identifier /// \param purge if true, delete all data and metadata files in the table - /// \return Status indicating the outcome of the operation. + /// \return Result 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; + virtual Result DropTable(const TableIdentifier& identifier, bool purge) = 0; /// \brief Rename a table /// diff --git a/src/iceberg/catalog/memory/in_memory_catalog.cc b/src/iceberg/catalog/memory/in_memory_catalog.cc index b3fd0060a..3b74610a3 100644 --- a/src/iceberg/catalog/memory/in_memory_catalog.cc +++ b/src/iceberg/catalog/memory/in_memory_catalog.cc @@ -63,10 +63,10 @@ class ICEBERG_EXPORT InMemoryNamespace { /// \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); + /// \return Result indicating whether the namespace was deleted. + /// Returns false if the namespace does not exist. + /// Returns error if the namespace is not empty. + Result DropNamespace(const Namespace& namespace_ident); /// \brief Retrieves the properties of the specified namespace. /// @@ -107,9 +107,9 @@ class ICEBERG_EXPORT InMemoryNamespace { /// \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); + /// \return Result indicating whether the table was unregistered. + /// Returns false if the table does not exist. + Result DropTable(const TableIdentifier& table_ident); /// \brief Checks if a table exists in the specified namespace. /// @@ -224,7 +224,7 @@ Status InMemoryNamespace::CreateNamespace( return {}; } -Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) { +Result InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) { if (namespace_ident.levels.empty()) { return InvalidArgument("namespace identifier is empty"); } @@ -238,7 +238,7 @@ Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) { const auto it = parentRs.value()->children_.find(to_delete); if (it == parentRs.value()->children_.end()) { - return NotFound("namespace {} is not found", to_delete); + return false; } const auto& target = it->second; @@ -247,7 +247,7 @@ Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) { } parentRs.value()->children_.erase(to_delete); - return {}; + return true; } Result> InMemoryNamespace::GetProperties( @@ -299,11 +299,15 @@ Status InMemoryNamespace::RegisterTable(const TableIdentifier& table_ident, return {}; } -Status InMemoryNamespace::UnregisterTable(const TableIdentifier& table_ident) { +Result InMemoryNamespace::DropTable(const TableIdentifier& table_ident) { const auto ns = GetNamespace(this, table_ident.ns); ICEBERG_RETURN_UNEXPECTED(ns); - ns.value()->table_metadata_locations_.erase(table_ident.name); - return {}; + const auto it = ns.value()->table_metadata_locations_.find(table_ident.name); + if (it == ns.value()->table_metadata_locations_.end()) { + return false; + } + ns.value()->table_metadata_locations_.erase(it); + return true; } Result InMemoryNamespace::TableExists(const TableIdentifier& table_ident) const { @@ -369,7 +373,7 @@ Result> InMemoryCatalog::ListNamespaces( return root_namespace_->ListNamespaces(ns); } -Status InMemoryCatalog::DropNamespace(const Namespace& ns) { +Result InMemoryCatalog::DropNamespace(const Namespace& ns) { std::unique_lock lock(mutex_); return root_namespace_->DropNamespace(ns); } @@ -453,10 +457,10 @@ Result InMemoryCatalog::TableExists(const TableIdentifier& identifier) con return root_namespace_->TableExists(identifier); } -Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) { +Result 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); + return root_namespace_->DropTable(identifier); } Status InMemoryCatalog::RenameTable(const TableIdentifier& from, diff --git a/src/iceberg/catalog/memory/in_memory_catalog.h b/src/iceberg/catalog/memory/in_memory_catalog.h index 22a596c10..7cafe7d51 100644 --- a/src/iceberg/catalog/memory/in_memory_catalog.h +++ b/src/iceberg/catalog/memory/in_memory_catalog.h @@ -57,7 +57,7 @@ class ICEBERG_EXPORT InMemoryCatalog Result> ListNamespaces(const Namespace& ns) const override; - Status DropNamespace(const Namespace& ns) override; + Result DropNamespace(const Namespace& ns) override; Result NamespaceExists(const Namespace& ns) const override; @@ -89,7 +89,7 @@ class ICEBERG_EXPORT InMemoryCatalog Result TableExists(const TableIdentifier& identifier) const override; - Status DropTable(const TableIdentifier& identifier, bool purge) override; + Result DropTable(const TableIdentifier& identifier, bool purge) override; Status RenameTable(const TableIdentifier& from, const TableIdentifier& to) override; diff --git a/src/iceberg/catalog/rest/http_client.cc b/src/iceberg/catalog/rest/http_client.cc index d1138b787..f170c0132 100644 --- a/src/iceberg/catalog/rest/http_client.cc +++ b/src/iceberg/catalog/rest/http_client.cc @@ -240,12 +240,13 @@ Result HttpClient::Head( } Result HttpClient::Delete( - const std::string& path, const std::unordered_map& headers, + const std::string& path, const std::unordered_map& params, + const std::unordered_map& headers, const ErrorHandler& error_handler) { cpr::Response response; { std::lock_guard guard(session_mutex_); - PrepareSession(path, headers); + PrepareSession(path, headers, params); response = session_->Delete(); } diff --git a/src/iceberg/catalog/rest/http_client.h b/src/iceberg/catalog/rest/http_client.h index 56c9f2902..84695effa 100644 --- a/src/iceberg/catalog/rest/http_client.h +++ b/src/iceberg/catalog/rest/http_client.h @@ -104,6 +104,7 @@ class ICEBERG_REST_EXPORT HttpClient { /// \brief Sends a DELETE request. Result Delete(const std::string& path, + const std::unordered_map& params, const std::unordered_map& headers, const ErrorHandler& error_handler); diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index fd8bea0bf..df47dd77d 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -185,14 +185,22 @@ Result> RestCatalog::GetNamespacePr return get_response.properties; } -Status RestCatalog::DropNamespace(const Namespace& ns) { +Result RestCatalog::DropNamespace(const Namespace& ns) { ICEBERG_RETURN_UNEXPECTED( CheckEndpoint(supported_endpoints_, Endpoint::DropNamespace())); ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns)); - ICEBERG_ASSIGN_OR_RAISE( - const auto response, - client_->Delete(path, /*headers=*/{}, *DropNamespaceErrorHandler::Instance())); - return {}; + + auto response_or_error = client_->Delete(path, /*params=*/{}, /*headers=*/{}, + *DropNamespaceErrorHandler::Instance()); + + if (!response_or_error.has_value()) { + // Catch NoSuchNamespaceException and return false + if (response_or_error.error().kind == ErrorKind::kNoSuchNamespace) { + return false; + } + ICEBERG_RETURN_UNEXPECTED(response_or_error); + } + return true; } Result RestCatalog::NamespaceExists(const Namespace& ns) const { @@ -212,9 +220,8 @@ Result RestCatalog::NamespaceExists(const Namespace& ns) const { auto response_or_error = client_->Head(path, /*headers=*/{}, *NamespaceErrorHandler::Instance()); if (!response_or_error.has_value()) { - const auto& error = response_or_error.error(); // catch NoSuchNamespaceException/404 and return false - if (error.kind == ErrorKind::kNoSuchNamespace) { + if (response_or_error.error().kind == ErrorKind::kNoSuchNamespace) { return false; } ICEBERG_RETURN_UNEXPECTED(response_or_error); @@ -294,9 +301,24 @@ Result> RestCatalog::StageCreateTable( return NotImplemented("Not implemented"); } -Status RestCatalog::DropTable([[maybe_unused]] const TableIdentifier& identifier, - [[maybe_unused]] bool purge) { - return NotImplemented("Not implemented"); +Result RestCatalog::DropTable(const TableIdentifier& identifier, bool purge) { + ICEBERG_RETURN_UNEXPECTED(CheckEndpoint(supported_endpoints_, Endpoint::DeleteTable())); + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier)); + + std::unordered_map params; + if (purge) { + params["purgeRequested"] = "true"; + } + auto response_or_error = + client_->Delete(path, params, /*headers=*/{}, *TableErrorHandler::Instance()); + if (!response_or_error.has_value()) { + const auto& error = response_or_error.error(); + if (error.kind == ErrorKind::kNoSuchTable) { + return false; + } + ICEBERG_RETURN_UNEXPECTED(response_or_error); + } + return true; } Result RestCatalog::TableExists( diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index a80965211..056144dbc 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -64,7 +64,7 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog, Result> GetNamespaceProperties( const Namespace& ns) const override; - Status DropNamespace(const Namespace& ns) override; + Result DropNamespace(const Namespace& ns) override; Result NamespaceExists(const Namespace& ns) const override; @@ -95,7 +95,7 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog, Status RenameTable(const TableIdentifier& from, const TableIdentifier& to) override; - Status DropTable(const TableIdentifier& identifier, bool purge) override; + Result DropTable(const TableIdentifier& identifier, bool purge) override; Result> LoadTable(const TableIdentifier& identifier) override; diff --git a/src/iceberg/test/mock_catalog.h b/src/iceberg/test/mock_catalog.h index 7873e6fe3..7b93e0013 100644 --- a/src/iceberg/test/mock_catalog.h +++ b/src/iceberg/test/mock_catalog.h @@ -48,7 +48,7 @@ class MockCatalog : public Catalog { (const std::unordered_set&)), (override)); - MOCK_METHOD(Status, DropNamespace, (const Namespace&), (override)); + MOCK_METHOD(Result, DropNamespace, (const Namespace&), (override)); MOCK_METHOD(Result, NamespaceExists, (const Namespace&), (const, override)); @@ -75,7 +75,7 @@ class MockCatalog : public Catalog { MOCK_METHOD(Result, TableExists, (const TableIdentifier&), (const, override)); - MOCK_METHOD(Status, DropTable, (const TableIdentifier&, bool), (override)); + MOCK_METHOD(Result, DropTable, (const TableIdentifier&, bool), (override)); MOCK_METHOD(Status, RenameTable, (const TableIdentifier&, const TableIdentifier&), (override)); diff --git a/src/iceberg/test/rest_catalog_test.cc b/src/iceberg/test/rest_catalog_test.cc index 0ff36807b..55f0e8ec5 100644 --- a/src/iceberg/test/rest_catalog_test.cc +++ b/src/iceberg/test/rest_catalog_test.cc @@ -358,6 +358,16 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) { EXPECT_FALSE(*exists_result); } +TEST_F(RestCatalogIntegrationTest, DropNonExistentNamespace) { + auto catalog = CreateAndVerifyCatalog(); + Namespace ns{.levels = {"nonexistent_namespace"}}; + auto drop_result = catalog->DropNamespace(ns); + + // Should return false (namespace didn't exist, nothing to drop) + ASSERT_THAT(drop_result, IsOk()); + EXPECT_FALSE(*drop_result); +} + TEST_F(RestCatalogIntegrationTest, CreateTable) { auto catalog = CreateAndVerifyCatalog(); @@ -435,4 +445,70 @@ TEST_F(RestCatalogIntegrationTest, LoadTable) { EXPECT_EQ(loaded_schema->fields()[1].name(), "data"); } +TEST_F(RestCatalogIntegrationTest, DropTable) { + auto catalog = CreateAndVerifyCatalog(); + + // Create namespace and table first + Namespace ns{.levels = {"test_drop_table"}}; + CreateAndVerifyNamespace(*catalog, ns); + + // Create table + auto schema = CreateDefaultSchema(); + auto partition_spec = CreateDefaultPartitionSpec(); + auto sort_order = CreateDefaultSortOrder(); + + TableIdentifier table_id{.ns = ns, .name = "table_to_drop"}; + std::unordered_map table_properties; + auto create_result = catalog->CreateTable(table_id, schema, partition_spec, sort_order, + "", table_properties); + ASSERT_THAT(create_result, IsOk()); + + // Drop the table + auto drop_result = catalog->DropTable(table_id, /*purge=*/false); + ASSERT_THAT(drop_result, IsOk()); + EXPECT_TRUE(*drop_result); + + // TODO(Feiyang Li):Verify table no longer exists +} + +TEST_F(RestCatalogIntegrationTest, DropTableWithPurge) { + auto catalog = CreateAndVerifyCatalog(); + + // Create namespace and table first + Namespace ns{.levels = {"test_drop_table_purge"}}; + CreateAndVerifyNamespace(*catalog, ns); + + // Create table + auto schema = CreateDefaultSchema(); + auto partition_spec = CreateDefaultPartitionSpec(); + auto sort_order = CreateDefaultSortOrder(); + + TableIdentifier table_id{.ns = ns, .name = "table_to_purge"}; + std::unordered_map table_properties; + auto create_result = catalog->CreateTable(table_id, schema, partition_spec, sort_order, + "", table_properties); + ASSERT_THAT(create_result, IsOk()); + + // Drop the table with purge=true + auto drop_result = catalog->DropTable(table_id, /*purge=*/true); + ASSERT_THAT(drop_result, IsOk()); + EXPECT_TRUE(*drop_result); + + // TODO(Feiyang Li):Verify table no longer exists +} + +TEST_F(RestCatalogIntegrationTest, DropNonExistentTable) { + auto catalog = CreateAndVerifyCatalog(); + // Create namespace + Namespace ns{.levels = {"test_drop_nonexistent"}}; + CreateAndVerifyNamespace(*catalog, ns); + + TableIdentifier table_id{.ns = ns, .name = "nonexistent_table"}; + auto drop_result = catalog->DropTable(table_id, /*purge=*/false); + + // Should return false (table didn't exist, nothing to drop) + ASSERT_THAT(drop_result, IsOk()); + EXPECT_FALSE(*drop_result); +} + } // namespace iceberg::rest From e53cf1b348f1cfee0e569f0f55b66331293152fb Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Thu, 25 Dec 2025 15:11:16 +0800 Subject: [PATCH 3/4] 1 --- src/iceberg/test/rest_catalog_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/iceberg/test/rest_catalog_test.cc b/src/iceberg/test/rest_catalog_test.cc index 55f0e8ec5..79639c194 100644 --- a/src/iceberg/test/rest_catalog_test.cc +++ b/src/iceberg/test/rest_catalog_test.cc @@ -164,7 +164,7 @@ class RestCatalogIntegrationTest : public ::testing::Test { auto partition_spec_result = PartitionSpec::Make(PartitionSpec::kInitialSpecId, {}, 0); EXPECT_THAT(partition_spec_result, IsOk()); - return std::shared_ptr(std::move(*partition_spec_result)); + return {std::move(*partition_spec_result)}; } // Helper function to create a default unsorted sort order @@ -172,7 +172,7 @@ class RestCatalogIntegrationTest : public ::testing::Test { auto sort_order_result = SortOrder::Make(SortOrder::kUnsortedOrderId, std::vector{}); EXPECT_THAT(sort_order_result, IsOk()); - return std::shared_ptr(std::move(*sort_order_result)); + return {std::move(*sort_order_result)}; } static inline std::unique_ptr docker_compose_; From 7b6079dcf8804e3c8345a1652b2124e8a4ee431b Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Thu, 25 Dec 2025 17:02:52 +0800 Subject: [PATCH 4/4] feat(rest):implement table exists --- src/iceberg/catalog/rest/rest_catalog.cc | 31 ++++- src/iceberg/test/rest_catalog_test.cc | 170 +++++++++++------------ 2 files changed, 110 insertions(+), 91 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index df47dd77d..469680f3c 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -321,9 +321,34 @@ Result RestCatalog::DropTable(const TableIdentifier& identifier, bool purg return true; } -Result RestCatalog::TableExists( - [[maybe_unused]] const TableIdentifier& identifier) const { - return NotImplemented("Not implemented"); +Result RestCatalog::TableExists(const TableIdentifier& identifier) const { + auto check = CheckEndpoint(supported_endpoints_, Endpoint::TableExists()); + if (!check.has_value()) { + // Fall back to LoadTable endpoint (GET) + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier)); + auto response_or_error = + client_->Get(path, /*params=*/{}, /*headers=*/{}, *TableErrorHandler::Instance()); + if (!response_or_error.has_value()) { + if (response_or_error.error().kind == ErrorKind::kNoSuchTable) { + return false; + } + ICEBERG_RETURN_UNEXPECTED(response_or_error); + } + // GET succeeded, table exists + return true; + } + + ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier)); + auto response_or_error = + client_->Head(path, /*headers=*/{}, *TableErrorHandler::Instance()); + if (!response_or_error.has_value()) { + // catch NoSuchTableException/404 and return false + if (response_or_error.error().kind == ErrorKind::kNoSuchTable) { + return false; + } + ICEBERG_RETURN_UNEXPECTED(response_or_error); + } + return true; } Status RestCatalog::RenameTable([[maybe_unused]] const TableIdentifier& from, diff --git a/src/iceberg/test/rest_catalog_test.cc b/src/iceberg/test/rest_catalog_test.cc index 79639c194..890adbd34 100644 --- a/src/iceberg/test/rest_catalog_test.cc +++ b/src/iceberg/test/rest_catalog_test.cc @@ -136,21 +136,6 @@ class RestCatalogIntegrationTest : public ::testing::Test { return RestCatalog::Make(*config, std::make_shared()); } - // Helper function to create and verify a catalog (asserts on failure) - std::shared_ptr CreateAndVerifyCatalog() { - auto catalog_result = CreateCatalog(); - EXPECT_THAT(catalog_result, IsOk()); - return catalog_result.value(); - } - - // Helper function to create and verify a namespace - void CreateAndVerifyNamespace( - RestCatalog& catalog, const Namespace& ns, - const std::unordered_map& properties = {}) { - auto status = catalog.CreateNamespace(ns, properties); - EXPECT_THAT(status, IsOk()); - } - // Helper function to create a default schema for testing std::shared_ptr CreateDefaultSchema() { return std::make_shared( @@ -221,7 +206,9 @@ TEST_F(RestCatalogIntegrationTest, FetchServerConfigDirect) { } TEST_F(RestCatalogIntegrationTest, ListNamespaces) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); Namespace root{.levels = {}}; auto result = catalog->ListNamespaces(root); @@ -230,11 +217,14 @@ TEST_F(RestCatalogIntegrationTest, ListNamespaces) { } TEST_F(RestCatalogIntegrationTest, CreateNamespace) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create a simple namespace Namespace ns{.levels = {"test_ns"}}; - CreateAndVerifyNamespace(*catalog, ns); + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); // Verify it was created by listing Namespace root{.levels = {}}; @@ -245,13 +235,16 @@ TEST_F(RestCatalogIntegrationTest, CreateNamespace) { } TEST_F(RestCatalogIntegrationTest, CreateNamespaceWithProperties) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create namespace with properties Namespace ns{.levels = {"test_ns_props"}}; std::unordered_map properties{ {"owner", "test_user"}, {"description", "Test namespace with properties"}}; - CreateAndVerifyNamespace(*catalog, ns, properties); + auto status = catalog->CreateNamespace(ns, properties); + EXPECT_THAT(status, IsOk()); // Verify properties were set auto props_result = catalog->GetNamespaceProperties(ns); @@ -261,15 +254,19 @@ TEST_F(RestCatalogIntegrationTest, CreateNamespaceWithProperties) { } TEST_F(RestCatalogIntegrationTest, CreateNestedNamespace) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create parent namespace Namespace parent{.levels = {"parent"}}; - CreateAndVerifyNamespace(*catalog, parent); + auto status = catalog->CreateNamespace(parent, {}); + EXPECT_THAT(status, IsOk()); // Create nested namespace Namespace child{.levels = {"parent", "child"}}; - CreateAndVerifyNamespace(*catalog, child); + status = catalog->CreateNamespace(child, {}); + EXPECT_THAT(status, IsOk()); // Verify nested namespace exists auto list_result = catalog->ListNamespaces(parent); @@ -279,13 +276,16 @@ TEST_F(RestCatalogIntegrationTest, CreateNestedNamespace) { } TEST_F(RestCatalogIntegrationTest, GetNamespaceProperties) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create namespace with properties Namespace ns{.levels = {"test_get_props"}}; std::unordered_map properties{{"key1", "value1"}, {"key2", "value2"}}; - CreateAndVerifyNamespace(*catalog, ns, properties); + auto status = catalog->CreateNamespace(ns, properties); + EXPECT_THAT(status, IsOk()); // Get properties auto props_result = catalog->GetNamespaceProperties(ns); @@ -295,7 +295,9 @@ TEST_F(RestCatalogIntegrationTest, GetNamespaceProperties) { } TEST_F(RestCatalogIntegrationTest, NamespaceExists) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Check non-existent namespace Namespace ns{.levels = {"non_existent"}}; @@ -304,28 +306,32 @@ TEST_F(RestCatalogIntegrationTest, NamespaceExists) { EXPECT_FALSE(*exists_result); // Create namespace - CreateAndVerifyNamespace(*catalog, ns); + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); // Check it now exists exists_result = catalog->NamespaceExists(ns); ASSERT_THAT(exists_result, IsOk()); - EXPECT_TRUE(*exists_result); + EXPECT_TRUE(exists_result.value()); } TEST_F(RestCatalogIntegrationTest, UpdateNamespaceProperties) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create namespace with initial properties Namespace ns{.levels = {"test_update"}}; std::unordered_map initial_props{{"key1", "value1"}, {"key2", "value2"}}; - CreateAndVerifyNamespace(*catalog, ns, initial_props); + auto status = catalog->CreateNamespace(ns, initial_props); + EXPECT_THAT(status, IsOk()); // Update properties: modify key1, add key3, remove key2 std::unordered_map updates{{"key1", "updated_value1"}, {"key3", "value3"}}; std::unordered_set removals{"key2"}; - auto status = catalog->UpdateNamespaceProperties(ns, updates, removals); + status = catalog->UpdateNamespaceProperties(ns, updates, removals); EXPECT_THAT(status, IsOk()); // Verify updated properties @@ -337,11 +343,14 @@ TEST_F(RestCatalogIntegrationTest, UpdateNamespaceProperties) { } TEST_F(RestCatalogIntegrationTest, DropNamespace) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create namespace Namespace ns{.levels = {"test_drop"}}; - CreateAndVerifyNamespace(*catalog, ns); + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); // Verify it exists auto exists_result = catalog->NamespaceExists(ns); @@ -349,27 +358,32 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) { EXPECT_TRUE(*exists_result); // Drop namespace - auto status = catalog->DropNamespace(ns); - EXPECT_THAT(status, IsOk()); + auto drop_result = catalog->DropNamespace(ns); + EXPECT_THAT(drop_result, IsOk()); // Verify it no longer exists exists_result = catalog->NamespaceExists(ns); ASSERT_THAT(exists_result, IsOk()); - EXPECT_FALSE(*exists_result); + EXPECT_FALSE(exists_result.value()); } TEST_F(RestCatalogIntegrationTest, DropNonExistentNamespace) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); + Namespace ns{.levels = {"nonexistent_namespace"}}; auto drop_result = catalog->DropNamespace(ns); // Should return false (namespace didn't exist, nothing to drop) ASSERT_THAT(drop_result, IsOk()); - EXPECT_FALSE(*drop_result); + EXPECT_FALSE(drop_result.value()); } TEST_F(RestCatalogIntegrationTest, CreateTable) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create nested namespace with properties Namespace ns{.levels = {"test_create_table", "apple", "ios"}}; @@ -377,9 +391,13 @@ TEST_F(RestCatalogIntegrationTest, CreateTable) { {"community", "apache"}}; // Create parent namespaces first - CreateAndVerifyNamespace(*catalog, Namespace{.levels = {"test_create_table"}}); - CreateAndVerifyNamespace(*catalog, Namespace{.levels = {"test_create_table", "apple"}}); - CreateAndVerifyNamespace(*catalog, ns, ns_properties); + auto status = catalog->CreateNamespace(Namespace{.levels = {"test_create_table"}}, {}); + EXPECT_THAT(status, IsOk()); + status = + catalog->CreateNamespace(Namespace{.levels = {"test_create_table", "apple"}}, {}); + EXPECT_THAT(status, IsOk()); + status = catalog->CreateNamespace(ns, ns_properties); + EXPECT_THAT(status, IsOk()); auto schema = CreateDefaultSchema(); auto partition_spec = CreateDefaultPartitionSpec(); @@ -407,11 +425,14 @@ TEST_F(RestCatalogIntegrationTest, CreateTable) { } TEST_F(RestCatalogIntegrationTest, LoadTable) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create namespace and table first Namespace ns{.levels = {"test_load_table"}}; - CreateAndVerifyNamespace(*catalog, ns); + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); // Create schema, partition spec, and sort order using helper functions auto schema = CreateDefaultSchema(); @@ -446,11 +467,14 @@ TEST_F(RestCatalogIntegrationTest, LoadTable) { } TEST_F(RestCatalogIntegrationTest, DropTable) { - auto catalog = CreateAndVerifyCatalog(); + auto catalog_result = CreateCatalog(); + ASSERT_THAT(catalog_result, IsOk()); + auto& catalog = catalog_result.value(); // Create namespace and table first Namespace ns{.levels = {"test_drop_table"}}; - CreateAndVerifyNamespace(*catalog, ns); + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); // Create table auto schema = CreateDefaultSchema(); @@ -463,52 +487,22 @@ TEST_F(RestCatalogIntegrationTest, DropTable) { "", table_properties); ASSERT_THAT(create_result, IsOk()); + // Verify table exists + auto load_result = catalog->TableExists(table_id); + ASSERT_THAT(load_result, IsOk()); + EXPECT_TRUE(load_result.value()); + // Drop the table + std::println("[DEBUG] About to call DropTable..."); auto drop_result = catalog->DropTable(table_id, /*purge=*/false); + std::println("[DEBUG] DropTable returned!"); ASSERT_THAT(drop_result, IsOk()); - EXPECT_TRUE(*drop_result); - - // TODO(Feiyang Li):Verify table no longer exists -} - -TEST_F(RestCatalogIntegrationTest, DropTableWithPurge) { - auto catalog = CreateAndVerifyCatalog(); - - // Create namespace and table first - Namespace ns{.levels = {"test_drop_table_purge"}}; - CreateAndVerifyNamespace(*catalog, ns); - - // Create table - auto schema = CreateDefaultSchema(); - auto partition_spec = CreateDefaultPartitionSpec(); - auto sort_order = CreateDefaultSortOrder(); - - TableIdentifier table_id{.ns = ns, .name = "table_to_purge"}; - std::unordered_map table_properties; - auto create_result = catalog->CreateTable(table_id, schema, partition_spec, sort_order, - "", table_properties); - ASSERT_THAT(create_result, IsOk()); - - // Drop the table with purge=true - auto drop_result = catalog->DropTable(table_id, /*purge=*/true); - ASSERT_THAT(drop_result, IsOk()); - EXPECT_TRUE(*drop_result); - - // TODO(Feiyang Li):Verify table no longer exists -} - -TEST_F(RestCatalogIntegrationTest, DropNonExistentTable) { - auto catalog = CreateAndVerifyCatalog(); - // Create namespace - Namespace ns{.levels = {"test_drop_nonexistent"}}; - CreateAndVerifyNamespace(*catalog, ns); - - TableIdentifier table_id{.ns = ns, .name = "nonexistent_table"}; - auto drop_result = catalog->DropTable(table_id, /*purge=*/false); + EXPECT_TRUE(drop_result.value()); - // Should return false (table didn't exist, nothing to drop) - ASSERT_THAT(drop_result, IsOk()); - EXPECT_FALSE(*drop_result); + // Verify table no longer exists + load_result = catalog->TableExists(table_id); + ASSERT_THAT(load_result, IsOk()); + EXPECT_FALSE(load_result.value()); } } // namespace iceberg::rest