Skip to content

Commit 6f802aa

Browse files
authored
refactor: improve error collector (#424)
Improve the usability of `ErrorCollector` class: - Refine function names to be consistent. - Add easy-to-use functions and macros.
1 parent bdf9989 commit 6f802aa

File tree

3 files changed

+64
-54
lines changed

3 files changed

+64
-54
lines changed

src/iceberg/table_metadata.cc

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
#include "iceberg/sort_order.h"
4444
#include "iceberg/table_properties.h"
4545
#include "iceberg/table_update.h"
46+
#include "iceberg/util/error_collector.h"
4647
#include "iceberg/util/gzip_internal.h"
4748
#include "iceberg/util/location_util.h"
4849
#include "iceberg/util/macros.h"
@@ -476,10 +477,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID() {
476477
TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
477478
std::string uuid_str(uuid);
478479

479-
// Validation: UUID cannot be empty
480-
if (uuid_str.empty()) {
481-
return AddError(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
482-
}
480+
ICEBERG_BUILDER_CHECK(!uuid_str.empty(), "Cannot assign empty UUID");
483481

484482
// Check if UUID is already set to the same value (no-op)
485483
if (StringUtils::EqualsIgnoreCase(impl_->metadata.table_uuid, uuid_str)) {
@@ -498,20 +496,15 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
498496
TableMetadataBuilder& TableMetadataBuilder::UpgradeFormatVersion(
499497
int8_t new_format_version) {
500498
// Check that the new format version is supported
501-
if (new_format_version > TableMetadata::kSupportedTableFormatVersion) {
502-
return AddError(
503-
ErrorKind::kInvalidArgument,
504-
std::format(
505-
"Cannot upgrade table to unsupported format version: v{} (supported: v{})",
506-
new_format_version, TableMetadata::kSupportedTableFormatVersion));
507-
}
499+
ICEBERG_BUILDER_CHECK(
500+
new_format_version <= TableMetadata::kSupportedTableFormatVersion,
501+
"Cannot upgrade table to unsupported format version: v{} (supported: v{})",
502+
new_format_version, TableMetadata::kSupportedTableFormatVersion);
508503

509504
// Check that we're not downgrading
510-
if (new_format_version < impl_->metadata.format_version) {
511-
return AddError(ErrorKind::kInvalidArgument,
512-
std::format("Cannot downgrade v{} table to v{}",
513-
impl_->metadata.format_version, new_format_version));
514-
}
505+
ICEBERG_BUILDER_CHECK(new_format_version >= impl_->metadata.format_version,
506+
"Cannot downgrade v{} table to v{}",
507+
impl_->metadata.format_version, new_format_version);
515508

516509
// No-op if the version is the same
517510
if (new_format_version == impl_->metadata.format_version) {
@@ -567,16 +560,15 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
567560

568561
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
569562
std::shared_ptr<SortOrder> order) {
570-
BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
563+
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
571564
return SetDefaultSortOrder(order_id);
572565
}
573566

574567
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t order_id) {
575568
if (order_id == -1) {
576-
if (!impl_->last_added_order_id.has_value()) {
577-
return AddError(ErrorKind::kInvalidArgument,
578-
"Cannot set last added sort order: no sort order has been added");
579-
}
569+
ICEBERG_BUILDER_CHECK(
570+
impl_->last_added_order_id.has_value(),
571+
"Cannot set last added sort order: no sort order has been added");
580572
return SetDefaultSortOrder(impl_->last_added_order_id.value());
581573
}
582574

@@ -638,7 +630,7 @@ Result<int32_t> TableMetadataBuilder::AddSortOrderInternal(const SortOrder& orde
638630

639631
TableMetadataBuilder& TableMetadataBuilder::AddSortOrder(
640632
std::shared_ptr<SortOrder> order) {
641-
BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
633+
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
642634
return *this;
643635
}
644636

src/iceberg/update/update_properties.cc

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "iceberg/table_properties.h"
3131
#include "iceberg/table_update.h"
3232
#include "iceberg/transaction.h"
33+
#include "iceberg/util/error_collector.h"
3334
#include "iceberg/util/macros.h"
3435

3536
namespace iceberg {
@@ -49,11 +50,9 @@ UpdateProperties::~UpdateProperties() = default;
4950

5051
UpdateProperties& UpdateProperties::Set(const std::string& key,
5152
const std::string& value) {
52-
if (removals_.contains(key)) {
53-
return AddError(
54-
ErrorKind::kInvalidArgument,
55-
std::format("Cannot set property '{}' that is already marked for removal", key));
56-
}
53+
ICEBERG_BUILDER_CHECK(!removals_.contains(key),
54+
"Cannot set property '{}' that is already marked for removal",
55+
key);
5756

5857
if (!TableProperties::reserved_properties().contains(key) ||
5958
key == TableProperties::kFormatVersion.key()) {
@@ -64,13 +63,9 @@ UpdateProperties& UpdateProperties::Set(const std::string& key,
6463
}
6564

6665
UpdateProperties& UpdateProperties::Remove(const std::string& key) {
67-
if (updates_.contains(key)) {
68-
return AddError(
69-
ErrorKind::kInvalidArgument,
70-
std::format("Cannot remove property '{}' that is already marked for update",
71-
key));
72-
}
73-
66+
ICEBERG_BUILDER_CHECK(!updates_.contains(key),
67+
"Cannot remove property '{}' that is already marked for update",
68+
key);
7469
removals_.insert(key);
7570
return *this;
7671
}

src/iceberg/util/error_collector.h

Lines changed: 43 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -30,30 +30,37 @@
3030

3131
namespace iceberg {
3232

33-
#define BUILDER_RETURN_IF_ERROR(result) \
33+
#define ICEBERG_BUILDER_RETURN_IF_ERROR(result) \
3434
if (auto&& result_name = result; !result_name) [[unlikely]] { \
3535
errors_.emplace_back(std::move(result_name.error())); \
3636
return *this; \
3737
}
3838

39-
#define BUILDER_ASSIGN_OR_RETURN_IMPL(result_name, lhs, rexpr) \
40-
auto&& result_name = (rexpr); \
41-
BUILDER_RETURN_IF_ERROR(result_name) \
39+
#define ICEBERG_BUILDER_ASSIGN_OR_RETURN_IMPL(result_name, lhs, rexpr) \
40+
auto&& result_name = (rexpr); \
41+
ICEBERG_BUILDER_RETURN_IF_ERROR(result_name) \
4242
lhs = std::move(result_name.value());
4343

44-
#define BUILDER_ASSIGN_OR_RETURN(lhs, rexpr) \
45-
BUILDER_ASSIGN_OR_RETURN_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
46-
rexpr)
44+
#define ICEBERG_BUILDER_ASSIGN_OR_RETURN(lhs, rexpr) \
45+
ICEBERG_BUILDER_ASSIGN_OR_RETURN_IMPL( \
46+
ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, rexpr)
4747

48-
/// \brief Base class for collecting validation errors in builder patterns
48+
#define ICEBERG_BUILDER_CHECK(expr, ...) \
49+
do { \
50+
if (!(expr)) [[unlikely]] { \
51+
return AddError(ErrorKind::kInvalidArgument, __VA_ARGS__); \
52+
} \
53+
} while (false)
54+
55+
/// \brief Base class for collecting errors in the builder pattern.
4956
///
50-
/// This class provides error accumulation functionality for builders that
51-
/// cannot throw exceptions. Builder methods can call AddError() to accumulate
52-
/// validation errors, and CheckErrors() returns all errors at once.
57+
/// This class equips builders with error accumulation capabilities to make it easy
58+
/// for method chaining. Builder methods should call AddError() to accumulate errors
59+
/// and call CheckErrors() before completing the build process.
5360
///
5461
/// Example usage:
5562
/// \code
56-
/// class MyBuilder : public ErrorCollectorBase {
63+
/// class MyBuilder : public ErrorCollector {
5764
/// public:
5865
/// MyBuilder& SetValue(int val) {
5966
/// if (val < 0) {
@@ -87,10 +94,13 @@ class ICEBERG_EXPORT ErrorCollector {
8794
///
8895
/// \param self Deduced reference to the derived class instance
8996
/// \param kind The kind of error
90-
/// \param message The error message
97+
/// \param fmt The format string
98+
/// \param args The arguments to format the message
9199
/// \return Reference to the derived class for method chaining
92-
auto& AddError(this auto& self, ErrorKind kind, std::string message) {
93-
self.errors_.emplace_back(kind, std::move(message));
100+
template <typename... Args>
101+
auto& AddError(this auto& self, ErrorKind kind, const std::format_string<Args...> fmt,
102+
Args&&... args) {
103+
self.errors_.emplace_back(kind, std::format(fmt, std::forward<Args>(args)...));
94104
return self;
95105
}
96106

@@ -107,15 +117,30 @@ class ICEBERG_EXPORT ErrorCollector {
107117
return self;
108118
}
109119

120+
/// \brief Add an unexpected result's error and return reference to derived class
121+
///
122+
/// Useful for cases like below:
123+
/// \code
124+
/// return AddError(InvalidArgument("Invalid value: {}", value));
125+
/// \endcode
126+
///
127+
/// \param self Deduced reference to the derived class instance
128+
/// \param err The unexpected result containing the error to add
129+
/// \return Reference to the derived class for method chaining
130+
auto& AddError(this auto& self, std::unexpected<Error> err) {
131+
self.errors_.push_back(std::move(err.error()));
132+
return self;
133+
}
134+
110135
/// \brief Check if any errors have been collected
111136
///
112137
/// \return true if there are accumulated errors
113-
[[nodiscard]] bool HasErrors() const { return !errors_.empty(); }
138+
[[nodiscard]] bool has_errors() const { return !errors_.empty(); }
114139

115140
/// \brief Get the number of errors collected
116141
///
117142
/// \return The count of accumulated errors
118-
[[nodiscard]] size_t ErrorCount() const { return errors_.size(); }
143+
[[nodiscard]] size_t error_count() const { return errors_.size(); }
119144

120145
/// \brief Check for accumulated errors and return them if any exist
121146
///
@@ -143,9 +168,7 @@ class ICEBERG_EXPORT ErrorCollector {
143168
void ClearErrors() { errors_.clear(); }
144169

145170
/// \brief Get read-only access to all collected errors
146-
///
147-
/// \return A const reference to the vector of errors
148-
[[nodiscard]] const std::vector<Error>& Errors() const { return errors_; }
171+
[[nodiscard]] const std::vector<Error>& errors() const { return errors_; }
149172

150173
protected:
151174
std::vector<Error> errors_;

0 commit comments

Comments
 (0)