This is an automated email from the ASF dual-hosted git repository.
gangwu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 6f802aa3 refactor: improve error collector (#424)
6f802aa3 is described below
commit 6f802aa33aba2641ef1ad0e4ced7d1443ee7158d
Author: Gang Wu <[email protected]>
AuthorDate: Mon Dec 22 13:49:08 2025 +0800
refactor: improve error collector (#424)
Improve the usability of `ErrorCollector` class:
- 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 e787b6fd..9bf6c530 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"
@@ -476,10 +477,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)) {
@@ -498,20 +496,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) {
@@ -567,16 +560,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());
}
@@ -638,7 +630,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;
}
diff --git a/src/iceberg/update/update_properties.cc
b/src/iceberg/update/update_properties.cc
index e502848c..f8a1d85d 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 48ea717e..a08665e8 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 <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;
}
@@ -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
///
@@ -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_;
