Skip to content

Commit 10c4e02

Browse files
HeartLinkedHeartLinked
committed
feat(rest):implement drop table
1 parent c0fdd0d commit 10c4e02

File tree

9 files changed

+141
-37
lines changed

9 files changed

+141
-37
lines changed

‎src/iceberg/catalog.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ class ICEBERG_EXPORT Catalog {
7575
/// \return Status::OK if dropped successfully;
7676
/// ErrorKind::kNoSuchNamespace if the namespace does not exist;
7777
/// ErrorKind::kNotAllowed if the namespace is not empty
78-
virtual Status DropNamespace(const Namespace& ns) = 0;
78+
virtual Result<bool> DropNamespace(const Namespace& ns) = 0;
7979

8080
/// \brief Check whether the namespace exists.
8181
///
@@ -160,10 +160,10 @@ class ICEBERG_EXPORT Catalog {
160160
///
161161
/// \param identifier a table identifier
162162
/// \param purge if true, delete all data and metadata files in the table
163-
/// \return Status indicating the outcome of the operation.
163+
/// \return Result<bool> indicating the outcome of the operation.
164164
/// - On success, the table was dropped (or did not exist).
165165
/// - On failure, contains error information.
166-
virtual Status DropTable(const TableIdentifier& identifier, bool purge) = 0;
166+
virtual Result<bool> DropTable(const TableIdentifier& identifier, bool purge) = 0;
167167

168168
/// \brief Rename a table
169169
///

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

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,10 @@ class ICEBERG_EXPORT InMemoryNamespace {
6363
/// \brief Deletes an existing namespace.
6464
///
6565
/// \param namespace_ident The namespace to delete.
66-
/// \return Status::OK if the namespace is deleted;
67-
/// ErrorKind::kNoSuchNamespace if the namespace does not exist;
68-
/// ErrorKind::kNotAllowed if the namespace is not empty.
69-
Status DropNamespace(const Namespace& namespace_ident);
66+
/// \return Result<bool> indicating whether the namespace was deleted.
67+
/// Returns false if the namespace does not exist.
68+
/// Returns error if the namespace is not empty.
69+
Result<bool> DropNamespace(const Namespace& namespace_ident);
7070

7171
/// \brief Retrieves the properties of the specified namespace.
7272
///
@@ -107,9 +107,9 @@ class ICEBERG_EXPORT InMemoryNamespace {
107107
/// \brief Unregisters a table from the specified namespace.
108108
///
109109
/// \param table_ident The identifier of the table to unregister.
110-
/// \return Status::OK if the table is removed;
111-
/// ErrorKind::kNoSuchTable if the table does not exist.
112-
Status UnregisterTable(const TableIdentifier& table_ident);
110+
/// \return Result<bool> indicating whether the table was unregistered.
111+
/// Returns false if the table does not exist.
112+
Result<bool> DropTable(const TableIdentifier& table_ident);
113113

114114
/// \brief Checks if a table exists in the specified namespace.
115115
///
@@ -224,7 +224,7 @@ Status InMemoryNamespace::CreateNamespace(
224224
return {};
225225
}
226226

227-
Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) {
227+
Result<bool> InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) {
228228
if (namespace_ident.levels.empty()) {
229229
return InvalidArgument("namespace identifier is empty");
230230
}
@@ -238,7 +238,7 @@ Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) {
238238

239239
const auto it = parentRs.value()->children_.find(to_delete);
240240
if (it == parentRs.value()->children_.end()) {
241-
return NotFound("namespace {} is not found", to_delete);
241+
return false;
242242
}
243243

244244
const auto& target = it->second;
@@ -247,7 +247,7 @@ Status InMemoryNamespace::DropNamespace(const Namespace& namespace_ident) {
247247
}
248248

249249
parentRs.value()->children_.erase(to_delete);
250-
return {};
250+
return true;
251251
}
252252

253253
Result<std::unordered_map<std::string, std::string>> InMemoryNamespace::GetProperties(
@@ -299,11 +299,15 @@ Status InMemoryNamespace::RegisterTable(const TableIdentifier& table_ident,
299299
return {};
300300
}
301301

302-
Status InMemoryNamespace::UnregisterTable(const TableIdentifier& table_ident) {
302+
Result<bool> InMemoryNamespace::DropTable(const TableIdentifier& table_ident) {
303303
const auto ns = GetNamespace(this, table_ident.ns);
304304
ICEBERG_RETURN_UNEXPECTED(ns);
305-
ns.value()->table_metadata_locations_.erase(table_ident.name);
306-
return {};
305+
const auto it = ns.value()->table_metadata_locations_.find(table_ident.name);
306+
if (it == ns.value()->table_metadata_locations_.end()) {
307+
return false;
308+
}
309+
ns.value()->table_metadata_locations_.erase(it);
310+
return true;
307311
}
308312

309313
Result<bool> InMemoryNamespace::TableExists(const TableIdentifier& table_ident) const {
@@ -369,7 +373,7 @@ Result<std::vector<Namespace>> InMemoryCatalog::ListNamespaces(
369373
return root_namespace_->ListNamespaces(ns);
370374
}
371375

372-
Status InMemoryCatalog::DropNamespace(const Namespace& ns) {
376+
Result<bool> InMemoryCatalog::DropNamespace(const Namespace& ns) {
373377
std::unique_lock lock(mutex_);
374378
return root_namespace_->DropNamespace(ns);
375379
}
@@ -453,10 +457,10 @@ Result<bool> InMemoryCatalog::TableExists(const TableIdentifier& identifier) con
453457
return root_namespace_->TableExists(identifier);
454458
}
455459

456-
Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) {
460+
Result<bool> InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) {
457461
std::unique_lock lock(mutex_);
458462
// TODO(Guotao): Delete all metadata files if purge is true.
459-
return root_namespace_->UnregisterTable(identifier);
463+
return root_namespace_->DropTable(identifier);
460464
}
461465

462466
Status InMemoryCatalog::RenameTable(const TableIdentifier& from,

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class ICEBERG_EXPORT InMemoryCatalog
5757

5858
Result<std::vector<Namespace>> ListNamespaces(const Namespace& ns) const override;
5959

60-
Status DropNamespace(const Namespace& ns) override;
60+
Result<bool> DropNamespace(const Namespace& ns) override;
6161

6262
Result<bool> NamespaceExists(const Namespace& ns) const override;
6363

@@ -89,7 +89,7 @@ class ICEBERG_EXPORT InMemoryCatalog
8989

9090
Result<bool> TableExists(const TableIdentifier& identifier) const override;
9191

92-
Status DropTable(const TableIdentifier& identifier, bool purge) override;
92+
Result<bool> DropTable(const TableIdentifier& identifier, bool purge) override;
9393

9494
Status RenameTable(const TableIdentifier& from, const TableIdentifier& to) override;
9595

‎src/iceberg/catalog/rest/http_client.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,12 +240,13 @@ Result<HttpResponse> HttpClient::Head(
240240
}
241241

242242
Result<HttpResponse> HttpClient::Delete(
243-
const std::string& path, const std::unordered_map<std::string, std::string>& headers,
243+
const std::string& path, const std::unordered_map<std::string, std::string>& params,
244+
const std::unordered_map<std::string, std::string>& headers,
244245
const ErrorHandler& error_handler) {
245246
cpr::Response response;
246247
{
247248
std::lock_guard guard(session_mutex_);
248-
PrepareSession(path, headers);
249+
PrepareSession(path, headers, params);
249250
response = session_->Delete();
250251
}
251252

‎src/iceberg/catalog/rest/http_client.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ class ICEBERG_REST_EXPORT HttpClient {
104104

105105
/// \brief Sends a DELETE request.
106106
Result<HttpResponse> Delete(const std::string& path,
107+
const std::unordered_map<std::string, std::string>& params,
107108
const std::unordered_map<std::string, std::string>& headers,
108109
const ErrorHandler& error_handler);
109110

‎src/iceberg/catalog/rest/rest_catalog.cc‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -185,14 +185,22 @@ Result<std::unordered_map<std::string, std::string>> RestCatalog::GetNamespacePr
185185
return get_response.properties;
186186
}
187187

188-
Status RestCatalog::DropNamespace(const Namespace& ns) {
188+
Result<bool> RestCatalog::DropNamespace(const Namespace& ns) {
189189
ICEBERG_RETURN_UNEXPECTED(
190190
CheckEndpoint(supported_endpoints_, Endpoint::DropNamespace()));
191191
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Namespace_(ns));
192-
ICEBERG_ASSIGN_OR_RAISE(
193-
const auto response,
194-
client_->Delete(path, /*headers=*/{}, *DropNamespaceErrorHandler::Instance()));
195-
return {};
192+
193+
auto response_or_error = client_->Delete(path, /*params=*/{}, /*headers=*/{},
194+
*DropNamespaceErrorHandler::Instance());
195+
196+
if (!response_or_error.has_value()) {
197+
// Catch NoSuchNamespaceException and return false
198+
if (response_or_error.error().kind == ErrorKind::kNoSuchNamespace) {
199+
return false;
200+
}
201+
ICEBERG_RETURN_UNEXPECTED(response_or_error);
202+
}
203+
return true;
196204
}
197205

198206
Result<bool> RestCatalog::NamespaceExists(const Namespace& ns) const {
@@ -212,9 +220,8 @@ Result<bool> RestCatalog::NamespaceExists(const Namespace& ns) const {
212220
auto response_or_error =
213221
client_->Head(path, /*headers=*/{}, *NamespaceErrorHandler::Instance());
214222
if (!response_or_error.has_value()) {
215-
const auto& error = response_or_error.error();
216223
// catch NoSuchNamespaceException/404 and return false
217-
if (error.kind == ErrorKind::kNoSuchNamespace) {
224+
if (response_or_error.error().kind == ErrorKind::kNoSuchNamespace) {
218225
return false;
219226
}
220227
ICEBERG_RETURN_UNEXPECTED(response_or_error);
@@ -294,9 +301,24 @@ Result<std::shared_ptr<Transaction>> RestCatalog::StageCreateTable(
294301
return NotImplemented("Not implemented");
295302
}
296303

297-
Status RestCatalog::DropTable([[maybe_unused]] const TableIdentifier& identifier,
298-
[[maybe_unused]] bool purge) {
299-
return NotImplemented("Not implemented");
304+
Result<bool> RestCatalog::DropTable(const TableIdentifier& identifier, bool purge) {
305+
ICEBERG_RETURN_UNEXPECTED(CheckEndpoint(supported_endpoints_, Endpoint::DeleteTable()));
306+
ICEBERG_ASSIGN_OR_RAISE(auto path, paths_->Table(identifier));
307+
308+
std::unordered_map<std::string, std::string> params;
309+
if (purge) {
310+
params["purgeRequested"] = "true";
311+
}
312+
auto response_or_error =
313+
client_->Delete(path, params, /*headers=*/{}, *TableErrorHandler::Instance());
314+
if (!response_or_error.has_value()) {
315+
const auto& error = response_or_error.error();
316+
if (error.kind == ErrorKind::kNoSuchTable) {
317+
return false;
318+
}
319+
ICEBERG_RETURN_UNEXPECTED(response_or_error);
320+
}
321+
return true;
300322
}
301323

302324
Result<bool> RestCatalog::TableExists(

‎src/iceberg/catalog/rest/rest_catalog.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog,
6464
Result<std::unordered_map<std::string, std::string>> GetNamespaceProperties(
6565
const Namespace& ns) const override;
6666

67-
Status DropNamespace(const Namespace& ns) override;
67+
Result<bool> DropNamespace(const Namespace& ns) override;
6868

6969
Result<bool> NamespaceExists(const Namespace& ns) const override;
7070

@@ -95,7 +95,7 @@ class ICEBERG_REST_EXPORT RestCatalog : public Catalog,
9595

9696
Status RenameTable(const TableIdentifier& from, const TableIdentifier& to) override;
9797

98-
Status DropTable(const TableIdentifier& identifier, bool purge) override;
98+
Result<bool> DropTable(const TableIdentifier& identifier, bool purge) override;
9999

100100
Result<std::shared_ptr<Table>> LoadTable(const TableIdentifier& identifier) override;
101101

‎src/iceberg/test/mock_catalog.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class MockCatalog : public Catalog {
4848
(const std::unordered_set<std::string>&)),
4949
(override));
5050

51-
MOCK_METHOD(Status, DropNamespace, (const Namespace&), (override));
51+
MOCK_METHOD(Result<bool>, DropNamespace, (const Namespace&), (override));
5252

5353
MOCK_METHOD(Result<bool>, NamespaceExists, (const Namespace&), (const, override));
5454

@@ -75,7 +75,7 @@ class MockCatalog : public Catalog {
7575

7676
MOCK_METHOD(Result<bool>, TableExists, (const TableIdentifier&), (const, override));
7777

78-
MOCK_METHOD(Status, DropTable, (const TableIdentifier&, bool), (override));
78+
MOCK_METHOD(Result<bool>, DropTable, (const TableIdentifier&, bool), (override));
7979

8080
MOCK_METHOD(Status, RenameTable, (const TableIdentifier&, const TableIdentifier&),
8181
(override));

‎src/iceberg/test/rest_catalog_test.cc‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,16 @@ TEST_F(RestCatalogIntegrationTest, DropNamespace) {
358358
EXPECT_FALSE(*exists_result);
359359
}
360360

361+
TEST_F(RestCatalogIntegrationTest, DropNonExistentNamespace) {
362+
auto catalog = CreateAndVerifyCatalog();
363+
Namespace ns{.levels = {"nonexistent_namespace"}};
364+
auto drop_result = catalog->DropNamespace(ns);
365+
366+
// Should return false (namespace didn't exist, nothing to drop)
367+
ASSERT_THAT(drop_result, IsOk());
368+
EXPECT_FALSE(*drop_result);
369+
}
370+
361371
TEST_F(RestCatalogIntegrationTest, CreateTable) {
362372
auto catalog = CreateAndVerifyCatalog();
363373

@@ -435,4 +445,70 @@ TEST_F(RestCatalogIntegrationTest, LoadTable) {
435445
EXPECT_EQ(loaded_schema->fields()[1].name(), "data");
436446
}
437447

448+
TEST_F(RestCatalogIntegrationTest, DropTable) {
449+
auto catalog = CreateAndVerifyCatalog();
450+
451+
// Create namespace and table first
452+
Namespace ns{.levels = {"test_drop_table"}};
453+
CreateAndVerifyNamespace(*catalog, ns);
454+
455+
// Create table
456+
auto schema = CreateDefaultSchema();
457+
auto partition_spec = CreateDefaultPartitionSpec();
458+
auto sort_order = CreateDefaultSortOrder();
459+
460+
TableIdentifier table_id{.ns = ns, .name = "table_to_drop"};
461+
std::unordered_map<std::string, std::string> table_properties;
462+
auto create_result = catalog->CreateTable(table_id, schema, partition_spec, sort_order,
463+
"", table_properties);
464+
ASSERT_THAT(create_result, IsOk());
465+
466+
// Drop the table
467+
auto drop_result = catalog->DropTable(table_id, /*purge=*/false);
468+
ASSERT_THAT(drop_result, IsOk());
469+
EXPECT_TRUE(*drop_result);
470+
471+
// TODO(Feiyang Li):Verify table no longer exists
472+
}
473+
474+
TEST_F(RestCatalogIntegrationTest, DropTableWithPurge) {
475+
auto catalog = CreateAndVerifyCatalog();
476+
477+
// Create namespace and table first
478+
Namespace ns{.levels = {"test_drop_table_purge"}};
479+
CreateAndVerifyNamespace(*catalog, ns);
480+
481+
// Create table
482+
auto schema = CreateDefaultSchema();
483+
auto partition_spec = CreateDefaultPartitionSpec();
484+
auto sort_order = CreateDefaultSortOrder();
485+
486+
TableIdentifier table_id{.ns = ns, .name = "table_to_purge"};
487+
std::unordered_map<std::string, std::string> table_properties;
488+
auto create_result = catalog->CreateTable(table_id, schema, partition_spec, sort_order,
489+
"", table_properties);
490+
ASSERT_THAT(create_result, IsOk());
491+
492+
// Drop the table with purge=true
493+
auto drop_result = catalog->DropTable(table_id, /*purge=*/true);
494+
ASSERT_THAT(drop_result, IsOk());
495+
EXPECT_TRUE(*drop_result);
496+
497+
// TODO(Feiyang Li):Verify table no longer exists
498+
}
499+
500+
TEST_F(RestCatalogIntegrationTest, DropNonExistentTable) {
501+
auto catalog = CreateAndVerifyCatalog();
502+
// Create namespace
503+
Namespace ns{.levels = {"test_drop_nonexistent"}};
504+
CreateAndVerifyNamespace(*catalog, ns);
505+
506+
TableIdentifier table_id{.ns = ns, .name = "nonexistent_table"};
507+
auto drop_result = catalog->DropTable(table_id, /*purge=*/false);
508+
509+
// Should return false (table didn't exist, nothing to drop)
510+
ASSERT_THAT(drop_result, IsOk());
511+
EXPECT_FALSE(*drop_result);
512+
}
513+
438514
} // namespace iceberg::rest

0 commit comments

Comments
 (0)