Skip to content

Commit 2a176fd

Browse files
committed
refactor: improve error collector
- Refine function names to be consistent. - Add easy-to-use functions and macros.
1 parent da91626 commit 2a176fd

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"
@@ -475,10 +476,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID() {
475476
TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
476477
std::string uuid_str(uuid);
477478

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

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

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

515508
// No-op if the version is the same
516509
if (new_format_version == impl_->metadata.format_version) {
@@ -566,16 +559,15 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
566559

567560
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
568561
std::shared_ptr<SortOrder> order) {
569-
BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
562+
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
570563
return SetDefaultSortOrder(order_id);
571564
}
572565

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

@@ -637,7 +629,7 @@ Result<int32_t> TableMetadataBuilder::AddSortOrderInternal(const SortOrder& orde
637629

638630
TableMetadataBuilder& TableMetadataBuilder::AddSortOrder(
639631
std::shared_ptr<SortOrder> order) {
640-
BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
632+
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
641633
return *this;
642634
}
643635

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)