Skip to content

Commit bbe7ba2

Browse files
gty404gty404
committed
Change the return result type of the method to use Result/Status
1 parent 9bb7ee9 commit bbe7ba2

File tree

5 files changed

+122
-93
lines changed

5 files changed

+122
-93
lines changed

‎src/iceberg/catalog.h‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,11 @@ class ICEBERG_EXPORT Catalog {
9090
/// \brief Check whether table exists
9191
///
9292
/// \param identifier a table identifier
93-
/// \return true if the table exists, false otherwise
94-
virtual bool TableExists(const TableIdentifier& identifier) const = 0;
93+
/// \return Status indicating success or failure.
94+
/// - On success, the table existence was successfully checked (actual existence
95+
/// may be inferred elsewhere).
96+
/// - On failure, contains error information.
97+
virtual Status TableExists(const TableIdentifier& identifier) const = 0;
9598

9699
/// \brief Drop a table; optionally delete data and metadata files
97100
///
@@ -100,8 +103,10 @@ class ICEBERG_EXPORT Catalog {
100103
///
101104
/// \param identifier a table identifier
102105
/// \param purge if true, delete all data and metadata files in the table
103-
/// \return true if the table was dropped, false if the table did not exist
104-
virtual bool DropTable(const TableIdentifier& identifier, bool purge) = 0;
106+
/// \return Status indicating the outcome of the operation.
107+
/// - On success, the table was dropped (or did not exist).
108+
/// - On failure, contains error information.
109+
virtual Status DropTable(const TableIdentifier& identifier, bool purge) = 0;
105110

106111
/// \brief Load a table
107112
///

‎src/iceberg/catalog/memory_catalog.cc‎

Lines changed: 73 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,19 @@
2424

2525
#include "iceberg/exception.h"
2626
#include "iceberg/table.h"
27+
#include "iceberg/util/macros.h"
2728

2829
namespace iceberg {
2930

3031
namespace {
3132

32-
InMemoryNamespace* GetNamespace(InMemoryNamespace* root,
33-
const Namespace& namespace_ident) {
33+
Result<InMemoryNamespace*> GetNamespace(InMemoryNamespace* root,
34+
const Namespace& namespace_ident) {
3435
return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident);
3536
}
3637

37-
const InMemoryNamespace* GetNamespace(const InMemoryNamespace* root,
38-
const Namespace& namespace_ident) {
38+
Result<const InMemoryNamespace*> GetNamespace(const InMemoryNamespace* root,
39+
const Namespace& namespace_ident) {
3940
return InMemoryNamespace::GetNamespaceImpl(root, namespace_ident);
4041
}
4142

@@ -60,10 +61,11 @@ Result<std::vector<TableIdentifier>> InMemoryCatalog::ListTables(
6061
const Namespace& ns) const {
6162
std::unique_lock lock(mutex_);
6263
const auto& table_names = root_namespace_->ListTables(ns);
64+
ICEBERG_RETURN_UNEXPECTED(table_names);
6365
std::vector<TableIdentifier> table_idents;
64-
table_idents.reserve(table_names.size());
66+
table_idents.reserve(table_names.value().size());
6567
std::ranges::transform(
66-
table_names, std::back_inserter(table_idents),
68+
table_names.value(), std::back_inserter(table_idents),
6769
[&ns](auto const& table_name) { return TableIdentifier(ns, table_name); });
6870
return table_idents;
6971
}
@@ -92,12 +94,12 @@ Result<std::shared_ptr<Transaction>> InMemoryCatalog::StageCreateTable(
9294
{.kind = ErrorKind::kNotImplemented, .message = "StageCreateTable"});
9395
}
9496

95-
bool InMemoryCatalog::TableExists(const TableIdentifier& identifier) const {
97+
Status InMemoryCatalog::TableExists(const TableIdentifier& identifier) const {
9698
std::unique_lock lock(mutex_);
9799
return root_namespace_->TableExists(identifier);
98100
}
99101

100-
bool InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) {
102+
Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge) {
101103
std::unique_lock lock(mutex_);
102104
// TODO(Guotao): Delete all metadata files if purge is true.
103105
return root_namespace_->UnregisterTable(identifier);
@@ -127,16 +129,19 @@ std::unique_ptr<TableBuilder> InMemoryCatalog::BuildTable(
127129
throw IcebergError("not implemented");
128130
}
129131

130-
bool InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const {
131-
return GetNamespace(this, namespace_ident) != nullptr;
132+
Status InMemoryNamespace::NamespaceExists(const Namespace& namespace_ident) const {
133+
const auto ns = GetNamespace(this, namespace_ident);
134+
ICEBERG_RETURN_UNEXPECTED(ns);
135+
return {};
132136
}
133137

134-
std::vector<std::string> InMemoryNamespace::ListChildrenNamespaces(
138+
Result<std::vector<std::string>> InMemoryNamespace::ListChildrenNamespaces(
135139
const std::optional<Namespace>& parent_namespace_ident) const {
136140
auto ns = this;
137141
if (parent_namespace_ident.has_value()) {
138-
ns = GetNamespace(this, *parent_namespace_ident);
139-
if (!ns) return {};
142+
const auto nsRs = GetNamespace(this, *parent_namespace_ident);
143+
ICEBERG_RETURN_UNEXPECTED(nsRs);
144+
ns = *nsRs;
140145
}
141146

142147
std::vector<std::string> names;
@@ -147,12 +152,15 @@ std::vector<std::string> InMemoryNamespace::ListChildrenNamespaces(
147152
return names;
148153
}
149154

150-
bool InMemoryNamespace::CreateNamespace(
155+
Status InMemoryNamespace::CreateNamespace(
151156
const Namespace& namespace_ident,
152157
const std::unordered_map<std::string, std::string>& properties) {
158+
if (namespace_ident.levels.empty()) {
159+
return InvalidArgument("namespace identifier is empty");
160+
}
161+
153162
auto ns = this;
154163
bool newly_created = false;
155-
156164
for (const auto& part_level : namespace_ident.levels) {
157165
if (auto it = ns->children_.find(part_level); it == ns->children_.end()) {
158166
ns = &ns->children_[part_level];
@@ -161,58 +169,62 @@ bool InMemoryNamespace::CreateNamespace(
161169
ns = &it->second;
162170
}
163171
}
164-
165172
if (!newly_created) {
166-
return false;
173+
return AlreadyExists("{}", namespace_ident.levels.back());
167174
}
168175

169176
ns->properties_ = properties;
170-
return true;
177+
return {};
171178
}
172179

173-
bool InMemoryNamespace::DeleteNamespace(const Namespace& namespace_ident) {
174-
if (namespace_ident.levels.empty()) return false;
180+
Status InMemoryNamespace::DeleteNamespace(const Namespace& namespace_ident) {
181+
if (namespace_ident.levels.empty()) {
182+
return InvalidArgument("namespace identifier is empty");
183+
}
175184

176185
auto parent_namespace_ident = namespace_ident;
177186
const auto to_delete = parent_namespace_ident.levels.back();
178187
parent_namespace_ident.levels.pop_back();
179188

180-
auto* parent = GetNamespace(this, parent_namespace_ident);
181-
if (!parent) return false;
189+
const auto parentRs = GetNamespace(this, parent_namespace_ident);
190+
ICEBERG_RETURN_UNEXPECTED(parentRs);
182191

183-
auto it = parent->children_.find(to_delete);
184-
if (it == parent->children_.end()) return false;
192+
const auto it = parentRs.value()->children_.find(to_delete);
193+
if (it == parentRs.value()->children_.end()) {
194+
return NotFound("namespace {} is not found", to_delete);
195+
}
185196

186197
const auto& target = it->second;
187198
if (!target.children_.empty() || !target.table_metadata_locations_.empty()) {
188-
return false;
199+
return NotAllowed("{} has other sub-namespaces and cannot be deleted", to_delete);
189200
}
190201

191-
return parent->children_.erase(to_delete) > 0;
202+
parentRs.value()->children_.erase(to_delete);
203+
return {};
192204
}
193205

194-
std::optional<std::unordered_map<std::string, std::string>>
195-
InMemoryNamespace::GetProperties(const Namespace& namespace_ident) const {
206+
Result<std::unordered_map<std::string, std::string>> InMemoryNamespace::GetProperties(
207+
const Namespace& namespace_ident) const {
196208
const auto ns = GetNamespace(this, namespace_ident);
197-
if (!ns) return std::nullopt;
198-
return ns->properties_;
209+
ICEBERG_RETURN_UNEXPECTED(ns);
210+
return ns.value()->properties_;
199211
}
200212

201-
bool InMemoryNamespace::ReplaceProperties(
213+
Status InMemoryNamespace::ReplaceProperties(
202214
const Namespace& namespace_ident,
203215
const std::unordered_map<std::string, std::string>& properties) {
204216
const auto ns = GetNamespace(this, namespace_ident);
205-
if (!ns) return false;
206-
ns->properties_ = properties;
207-
return true;
217+
ICEBERG_RETURN_UNEXPECTED(ns);
218+
ns.value()->properties_ = properties;
219+
return {};
208220
}
209221

210-
std::vector<std::string> InMemoryNamespace::ListTables(
222+
Result<std::vector<std::string>> InMemoryNamespace::ListTables(
211223
const Namespace& namespace_ident) const {
212224
const auto ns = GetNamespace(this, namespace_ident);
213-
if (!ns) return {};
225+
ICEBERG_RETURN_UNEXPECTED(ns);
214226

215-
const auto& locations = ns->table_metadata_locations_;
227+
const auto& locations = ns.value()->table_metadata_locations_;
216228
std::vector<std::string> table_names;
217229
table_names.reserve(locations.size());
218230

@@ -223,33 +235,41 @@ std::vector<std::string> InMemoryNamespace::ListTables(
223235
return table_names;
224236
}
225237

226-
bool InMemoryNamespace::RegisterTable(TableIdentifier const& table_ident,
227-
const std::string& metadata_location) {
238+
Status InMemoryNamespace::RegisterTable(TableIdentifier const& table_ident,
239+
const std::string& metadata_location) {
228240
const auto ns = GetNamespace(this, table_ident.ns);
229-
if (!ns) return false;
230-
if (ns->table_metadata_locations_.contains(table_ident.name)) return false;
231-
ns->table_metadata_locations_[table_ident.name] = metadata_location;
232-
return true;
241+
ICEBERG_RETURN_UNEXPECTED(ns);
242+
if (ns.value()->table_metadata_locations_.contains(table_ident.name)) {
243+
return AlreadyExists("{} already exists", table_ident.name);
244+
}
245+
ns.value()->table_metadata_locations_[table_ident.name] = metadata_location;
246+
return {};
233247
}
234248

235-
bool InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) {
249+
Status InMemoryNamespace::UnregisterTable(TableIdentifier const& table_ident) {
236250
const auto ns = GetNamespace(this, table_ident.ns);
237-
if (!ns) return false;
238-
return ns->table_metadata_locations_.erase(table_ident.name) > 0;
251+
ICEBERG_RETURN_UNEXPECTED(ns);
252+
ns.value()->table_metadata_locations_.erase(table_ident.name);
253+
return {};
239254
}
240255

241-
bool InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const {
256+
Status InMemoryNamespace::TableExists(TableIdentifier const& table_ident) const {
242257
const auto ns = GetNamespace(this, table_ident.ns);
243-
if (!ns) return false;
244-
return ns->table_metadata_locations_.contains(table_ident.name);
258+
ICEBERG_RETURN_UNEXPECTED(ns);
259+
if (!ns.value()->table_metadata_locations_.contains(table_ident.name)) {
260+
return NotFound("{} does not exist", table_ident.name);
261+
}
262+
return {};
245263
}
246264

247-
std::optional<std::string> InMemoryNamespace::GetTableMetadataLocation(
265+
Result<std::string> InMemoryNamespace::GetTableMetadataLocation(
248266
TableIdentifier const& table_ident) const {
249267
const auto ns = GetNamespace(this, table_ident.ns);
250-
if (!ns) return std::nullopt;
251-
const auto it = ns->table_metadata_locations_.find(table_ident.name);
252-
if (it == ns->table_metadata_locations_.end()) return std::nullopt;
268+
ICEBERG_RETURN_UNEXPECTED(ns);
269+
const auto it = ns.value()->table_metadata_locations_.find(table_ident.name);
270+
if (it == ns.value()->table_metadata_locations_.end()) {
271+
return NotFound("{} does not exist", table_ident.name);
272+
}
253273
return it->second;
254274
}
255275
} // namespace iceberg

0 commit comments

Comments
 (0)