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 eeffffd26..469680f3c 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,14 +301,54 @@ 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( - [[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, @@ -309,9 +356,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/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 fb1f70610..890adbd34 100644 --- a/src/iceberg/test/rest_catalog_test.cc +++ b/src/iceberg/test/rest_catalog_test.cc @@ -136,6 +136,30 @@ class RestCatalogIntegrationTest : public ::testing::Test { return RestCatalog::Make(*config, std::make_shared()); } + // 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::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::move(*sort_order_result)}; + } + static inline std::unique_ptr docker_compose_; }; @@ -288,7 +312,7 @@ TEST_F(RestCatalogIntegrationTest, NamespaceExists) { // 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) { @@ -334,13 +358,26 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) { EXPECT_TRUE(*exists_result); // Drop namespace - 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_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.value()); } TEST_F(RestCatalogIntegrationTest, CreateTable) { @@ -362,22 +399,9 @@ TEST_F(RestCatalogIntegrationTest, CreateTable) { 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); - - // 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 +424,85 @@ TEST_F(RestCatalogIntegrationTest, CreateTable) { HasErrorMessage("Table already exists: test_create_table.apple.ios.t1")); } +TEST_F(RestCatalogIntegrationTest, LoadTable) { + 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"}}; + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); + + // 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"); +} + +TEST_F(RestCatalogIntegrationTest, DropTable) { + 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"}}; + auto status = catalog->CreateNamespace(ns, {}); + EXPECT_THAT(status, IsOk()); + + // 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()); + + // 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.value()); + + // Verify table no longer exists + load_result = catalog->TableExists(table_id); + ASSERT_THAT(load_result, IsOk()); + EXPECT_FALSE(load_result.value()); +} + } // namespace iceberg::rest