Skip to content

refactor: improve error collector #424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wgtmac merged 1 commit into from
Dec 22, 2025
Merged

refactor: improve error collector #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 14 additions & 22 deletions src/iceberg/table_metadata.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "iceberg/sort_order.h"
#include "iceberg/table_properties.h"
#include "iceberg/table_update.h"
#include "iceberg/util/error_collector.h"
#include "iceberg/util/gzip_internal.h"
#include "iceberg/util/location_util.h"
#include "iceberg/util/macros.h"
Expand Down Expand Up @@ -475,10 +476,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID() {
TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
std::string uuid_str(uuid);

// Validation: UUID cannot be empty
if (uuid_str.empty()) {
return AddError(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
}
ICEBERG_BUILDER_CHECK(!uuid_str.empty(), "Cannot assign empty UUID");

// Check if UUID is already set to the same value (no-op)
if (StringUtils::EqualsIgnoreCase(impl_->metadata.table_uuid, uuid_str)) {
Expand All @@ -497,20 +495,15 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
TableMetadataBuilder& TableMetadataBuilder::UpgradeFormatVersion(
int8_t new_format_version) {
// Check that the new format version is supported
if (new_format_version > TableMetadata::kSupportedTableFormatVersion) {
return AddError(
ErrorKind::kInvalidArgument,
std::format(
"Cannot upgrade table to unsupported format version: v{} (supported: v{})",
new_format_version, TableMetadata::kSupportedTableFormatVersion));
}
ICEBERG_BUILDER_CHECK(
new_format_version <= TableMetadata::kSupportedTableFormatVersion,
"Cannot upgrade table to unsupported format version: v{} (supported: v{})",
new_format_version, TableMetadata::kSupportedTableFormatVersion);

// Check that we're not downgrading
if (new_format_version < impl_->metadata.format_version) {
return AddError(ErrorKind::kInvalidArgument,
std::format("Cannot downgrade v{} table to v{}",
impl_->metadata.format_version, new_format_version));
}
ICEBERG_BUILDER_CHECK(new_format_version >= impl_->metadata.format_version,
"Cannot downgrade v{} table to v{}",
impl_->metadata.format_version, new_format_version);

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

TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
return SetDefaultSortOrder(order_id);
}

TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t order_id) {
if (order_id == -1) {
if (!impl_->last_added_order_id.has_value()) {
return AddError(ErrorKind::kInvalidArgument,
"Cannot set last added sort order: no sort order has been added");
}
ICEBERG_BUILDER_CHECK(
impl_->last_added_order_id.has_value(),
"Cannot set last added sort order: no sort order has been added");
return SetDefaultSortOrder(impl_->last_added_order_id.value());
}

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

TableMetadataBuilder& TableMetadataBuilder::AddSortOrder(
std::shared_ptr<SortOrder> order) {
BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order));
return *this;
}

Expand Down
19 changes: 7 additions & 12 deletions src/iceberg/update/update_properties.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "iceberg/table_properties.h"
#include "iceberg/table_update.h"
#include "iceberg/transaction.h"
#include "iceberg/util/error_collector.h"
#include "iceberg/util/macros.h"

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

UpdateProperties& UpdateProperties::Set(const std::string& key,
const std::string& value) {
if (removals_.contains(key)) {
return AddError(
ErrorKind::kInvalidArgument,
std::format("Cannot set property '{}' that is already marked for removal", key));
}
ICEBERG_BUILDER_CHECK(!removals_.contains(key),
"Cannot set property '{}' that is already marked for removal",
key);

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

UpdateProperties& UpdateProperties::Remove(const std::string& key) {
if (updates_.contains(key)) {
return AddError(
ErrorKind::kInvalidArgument,
std::format("Cannot remove property '{}' that is already marked for update",
key));
}

ICEBERG_BUILDER_CHECK(!updates_.contains(key),
"Cannot remove property '{}' that is already marked for update",
key);
removals_.insert(key);
return *this;
}
Expand Down
63 changes: 43 additions & 20 deletions src/iceberg/util/error_collector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,30 +30,37 @@

namespace iceberg {

#define BUILDER_RETURN_IF_ERROR(result) \
#define ICEBERG_BUILDER_RETURN_IF_ERROR(result) \
if (auto&& result_name = result; !result_name) [[unlikely]] { \
errors_.emplace_back(std::move(result_name.error())); \
return *this; \
}

#define BUILDER_ASSIGN_OR_RETURN_IMPL(result_name, lhs, rexpr) \
auto&& result_name = (rexpr); \
BUILDER_RETURN_IF_ERROR(result_name) \
#define ICEBERG_BUILDER_ASSIGN_OR_RETURN_IMPL(result_name, lhs, rexpr) \
auto&& result_name = (rexpr); \
ICEBERG_BUILDER_RETURN_IF_ERROR(result_name) \
lhs = std::move(result_name.value());

#define BUILDER_ASSIGN_OR_RETURN(lhs, rexpr) \
BUILDER_ASSIGN_OR_RETURN_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
rexpr)
#define ICEBERG_BUILDER_ASSIGN_OR_RETURN(lhs, rexpr) \
ICEBERG_BUILDER_ASSIGN_OR_RETURN_IMPL( \
ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, rexpr)

/// \brief Base class for collecting validation errors in builder patterns
#define ICEBERG_BUILDER_CHECK(expr, ...) \
do { \
if (!(expr)) [[unlikely]] { \
return AddError(ErrorKind::kInvalidArgument, __VA_ARGS__); \
} \
} while (false)

/// \brief Base class for collecting errors in the builder pattern.
///
/// This class provides error accumulation functionality for builders that
/// cannot throw exceptions. Builder methods can call AddError() to accumulate
/// validation errors, and CheckErrors() returns all errors at once.
/// This class equips builders with error accumulation capabilities to make it easy
/// for method chaining. Builder methods should call AddError() to accumulate errors
/// and call CheckErrors() before completing the build process.
///
/// Example usage:
/// \code
/// class MyBuilder : public ErrorCollectorBase {
/// class MyBuilder : public ErrorCollector {
/// public:
/// MyBuilder& SetValue(int val) {
/// if (val < 0) {
Expand Down Expand Up @@ -87,10 +94,13 @@ class ICEBERG_EXPORT ErrorCollector {
///
/// \param self Deduced reference to the derived class instance
/// \param kind The kind of error
/// \param message The error message
/// \param fmt The format string
/// \param args The arguments to format the message
/// \return Reference to the derived class for method chaining
auto& AddError(this auto& self, ErrorKind kind, std::string message) {
self.errors_.emplace_back(kind, std::move(message));
template <typename... Args>
auto& AddError(this auto& self, ErrorKind kind, const std::format_string<Args...> fmt,
Args&&... args) {
self.errors_.emplace_back(kind, std::format(fmt, std::forward<Args>(args)...));
return self;
}

Expand All @@ -107,15 +117,30 @@ class ICEBERG_EXPORT ErrorCollector {
return self;
}

/// \brief Add an unexpected result's error and return reference to derived class
///
/// Useful for cases like below:
/// \code
/// return AddError(InvalidArgument("Invalid value: {}", value));
/// \endcode
///
/// \param self Deduced reference to the derived class instance
/// \param err The unexpected result containing the error to add
/// \return Reference to the derived class for method chaining
auto& AddError(this auto& self, std::unexpected<Error> err) {
self.errors_.push_back(std::move(err.error()));
return self;
}

/// \brief Check if any errors have been collected
///
/// \return true if there are accumulated errors
[[nodiscard]] bool HasErrors() const { return !errors_.empty(); }
[[nodiscard]] bool has_errors() const { return !errors_.empty(); }

/// \brief Get the number of errors collected
///
/// \return The count of accumulated errors
[[nodiscard]] size_t ErrorCount() const { return errors_.size(); }
[[nodiscard]] size_t error_count() const { return errors_.size(); }

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

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

protected:
std::vector<Error> errors_;
Expand Down
Loading