From 2a176fd1772f9f71f1a0e5d33e9cd5c9b57e08c3 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Fri, 19 Dec 2025 16:52:00 +0800 Subject: [PATCH] refactor: improve error collector - Refine function names to be consistent. - Add easy-to-use functions and macros. --- src/iceberg/table_metadata.cc | 36 ++++++-------- src/iceberg/update/update_properties.cc | 19 +++----- src/iceberg/util/error_collector.h | 63 +++++++++++++++++-------- 3 files changed, 64 insertions(+), 54 deletions(-) diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 61bb8e089..0b69c98c4 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -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" @@ -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)) { @@ -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) { @@ -566,16 +559,15 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas( TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder( std::shared_ptr 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()); } @@ -637,7 +629,7 @@ Result TableMetadataBuilder::AddSortOrderInternal(const SortOrder& orde TableMetadataBuilder& TableMetadataBuilder::AddSortOrder( std::shared_ptr order) { - BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order)); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(*order)); return *this; } diff --git a/src/iceberg/update/update_properties.cc b/src/iceberg/update/update_properties.cc index e502848c9..f8a1d85d5 100644 --- a/src/iceberg/update/update_properties.cc +++ b/src/iceberg/update/update_properties.cc @@ -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 { @@ -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()) { @@ -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; } diff --git a/src/iceberg/util/error_collector.h b/src/iceberg/util/error_collector.h index 48ea717eb..a08665e82 100644 --- a/src/iceberg/util/error_collector.h +++ b/src/iceberg/util/error_collector.h @@ -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) { @@ -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 + auto& AddError(this auto& self, ErrorKind kind, const std::format_string fmt, + Args&&... args) { + self.errors_.emplace_back(kind, std::format(fmt, std::forward(args)...)); return self; } @@ -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 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 /// @@ -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& Errors() const { return errors_; } + [[nodiscard]] const std::vector& errors() const { return errors_; } protected: std::vector errors_;