Skip to content

Commit 7bb446d

Browse files
committed
polish design
1 parent 37045cb commit 7bb446d

File tree

5 files changed

+44
-102
lines changed

5 files changed

+44
-102
lines changed

src/iceberg/pending_update.h

Lines changed: 1 addition & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -73,33 +73,9 @@ class ICEBERG_EXPORT PendingUpdate {
7373
/// Apply() can be used to validate and inspect the uncommitted changes before
7474
/// committing. Commit() applies the changes and commits them to the table.
7575
///
76-
/// Error Collection Pattern:
77-
/// Builder methods in subclasses should use AddError() to collect validation
78-
/// errors instead of returning immediately. The accumulated errors will be
79-
/// returned by Apply() or Commit() when they are called. This allows users
80-
/// to see all validation errors at once rather than fixing them one by one.
81-
///
82-
/// Example usage in a subclass:
83-
/// \code
84-
/// MyUpdate& SetProperty(std::string_view key, std::string_view value) {
85-
/// if (key.empty()) {
86-
/// AddError(ErrorKind::kInvalidArgument, "Property key cannot be empty");
87-
/// return *this;
88-
/// }
89-
/// // ... continue with normal logic
90-
/// return *this;
91-
/// }
92-
///
93-
/// Result<T> Apply() override {
94-
/// // Check for accumulated errors first
95-
/// ICEBERG_RETURN_IF_ERROR(CheckErrors());
96-
/// // ... proceed with apply logic
97-
/// }
98-
/// \endcode
99-
///
10076
/// \tparam T The type of result returned by Apply()
10177
template <typename T>
102-
class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
78+
class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate, public ErrorCollector {
10379
public:
10480
~PendingUpdateTyped() override = default;
10581

@@ -114,49 +90,6 @@ class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate {
11490

11591
protected:
11692
PendingUpdateTyped() = default;
117-
118-
/// \brief Add a validation error to be returned later
119-
///
120-
/// Errors are accumulated and will be returned by Apply() or Commit().
121-
/// This allows builder methods to continue and collect all errors rather
122-
/// than failing fast on the first error.
123-
///
124-
/// \param kind The kind of error
125-
/// \param message The error message
126-
void AddError(ErrorKind kind, std::string message) {
127-
error_collector_.AddError(kind, std::move(message));
128-
}
129-
130-
/// \brief Add an existing error object to be returned later
131-
///
132-
/// Useful when propagating errors from other components.
133-
///
134-
/// \param err The error to add
135-
void AddError(Error err) { error_collector_.AddError(std::move(err)); }
136-
137-
/// \brief Check if any errors have been collected
138-
///
139-
/// \return true if there are accumulated errors
140-
bool HasErrors() const { return error_collector_.HasErrors(); }
141-
142-
/// \brief Check for accumulated errors and return them if any exist
143-
///
144-
/// This should be called at the beginning of Apply() or Commit() to
145-
/// return all accumulated validation errors.
146-
///
147-
/// \return Status::OK if no errors, or a ValidationFailed error with
148-
/// all accumulated error messages
149-
Status CheckErrors() const { return error_collector_.CheckErrors(); }
150-
151-
/// \brief Clear all accumulated errors
152-
///
153-
/// This can be useful for resetting the error state in tests or
154-
/// when reusing a builder instance.
155-
void ClearErrors() { error_collector_.ClearErrors(); }
156-
157-
private:
158-
// Error collection (since builder methods return *this and cannot throw)
159-
ErrorCollector error_collector_;
16093
};
16194

16295
} // namespace iceberg

src/iceberg/table_metadata.cc

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
#include "iceberg/snapshot.h"
3737
#include "iceberg/sort_order.h"
3838
#include "iceberg/table_update.h"
39-
#include "iceberg/util/error_collector.h"
4039
#include "iceberg/util/gzip_internal.h"
4140
#include "iceberg/util/macros.h"
4241
#include "iceberg/util/uuid.h"
@@ -276,9 +275,6 @@ struct TableMetadataBuilder::Impl {
276275
// Change tracking
277276
std::vector<std::unique_ptr<TableUpdate>> changes;
278277

279-
// Error collection (since methods return *this and cannot throw)
280-
ErrorCollector error_collector;
281-
282278
// Metadata location tracking
283279
std::optional<std::string> metadata_location;
284280
std::optional<std::string> previous_metadata_location;
@@ -349,9 +345,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
349345

350346
// Validation: UUID cannot be empty
351347
if (uuid_str.empty()) {
352-
impl_->error_collector.AddError(ErrorKind::kInvalidArgument,
353-
"Cannot assign empty UUID");
354-
return *this;
348+
return AddError(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
355349
}
356350

357351
// Check if UUID is already set to the same value (no-op)
@@ -453,7 +447,7 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSnapshots(
453447
throw IcebergError(std::format("{} not implemented", __FUNCTION__));
454448
}
455449

456-
TableMetadataBuilder& TableMetadataBuilder::suppressHistoricalSnapshots() {
450+
TableMetadataBuilder& TableMetadataBuilder::SuppressHistoricalSnapshots() {
457451
throw IcebergError(std::format("{} not implemented", __FUNCTION__));
458452
}
459453

@@ -501,7 +495,7 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveEncryptionKey(std::string_view
501495

502496
Result<std::unique_ptr<TableMetadata>> TableMetadataBuilder::Build() {
503497
// 1. Check for accumulated errors
504-
ICEBERG_RETURN_UNEXPECTED(impl_->error_collector.CheckErrors());
498+
ICEBERG_RETURN_UNEXPECTED(CheckErrors());
505499

506500
// 2. Validate metadata consistency through TableMetadata#Validate
507501

src/iceberg/table_metadata.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030

3131
#include "iceberg/iceberg_export.h"
3232
#include "iceberg/type_fwd.h"
33+
#include "iceberg/util/error_collector.h"
3334
#include "iceberg/util/lazy.h"
3435
#include "iceberg/util/timepoint.h"
3536

@@ -193,7 +194,7 @@ ICEBERG_EXPORT std::string ToString(const MetadataLogEntry& entry);
193194
/// If a modification violates Iceberg table constraints (e.g., setting a current
194195
/// schema ID that does not exist), an error will be recorded and returned when
195196
/// Build() is called.
196-
class ICEBERG_EXPORT TableMetadataBuilder {
197+
class ICEBERG_EXPORT TableMetadataBuilder : public ErrorCollector {
197198
public:
198199
/// \brief Create a builder for a new table
199200
///
@@ -353,7 +354,7 @@ class ICEBERG_EXPORT TableMetadataBuilder {
353354
/// RemoveSnapshot changes are created. A snapshot is historical if no ref directly
354355
/// references its ID.
355356
/// \return Reference to this builder for method chaining
356-
TableMetadataBuilder& suppressHistoricalSnapshots();
357+
TableMetadataBuilder& SuppressHistoricalSnapshots();
357358

358359
/// \brief Set table statistics
359360
///

src/iceberg/util/error_collector.h

Lines changed: 36 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,74 +20,87 @@
2020
#pragma once
2121

2222
/// \file iceberg/util/error_collector.h
23-
/// Utility for collecting validation errors in builder patterns
23+
/// Base class for collecting validation errors in builder patterns
2424

2525
#include <string>
2626
#include <vector>
2727

28+
#include "iceberg/iceberg_export.h"
2829
#include "iceberg/result.h"
2930

3031
namespace iceberg {
3132

32-
/// \brief Utility class for collecting validation errors in builder patterns
33+
/// \brief Base class for collecting validation errors in builder patterns
3334
///
3435
/// This class provides error accumulation functionality for builders that
3536
/// cannot throw exceptions. Builder methods can call AddError() to accumulate
3637
/// validation errors, and CheckErrors() returns all errors at once.
3738
///
38-
/// This allows users to see all validation errors at once rather than fixing
39-
/// them one by one (fail-slow instead of fail-fast).
40-
///
4139
/// Example usage:
4240
/// \code
43-
/// class MyBuilder {
44-
/// protected:
45-
/// ErrorCollector errors_;
46-
///
41+
/// class MyBuilder : public ErrorCollectorBase {
42+
/// public:
4743
/// MyBuilder& SetValue(int val) {
4844
/// if (val < 0) {
49-
/// errors_.AddError(ErrorKind::kInvalidArgument, "Value must be non-negative");
50-
/// return *this;
45+
/// return AddError(ErrorKind::kInvalidArgument, "Value must be non-negative");
5146
/// }
5247
/// value_ = val;
5348
/// return *this;
5449
/// }
5550
///
5651
/// Result<MyObject> Build() {
57-
/// ICEBERG_RETURN_UNEXPECTED(errors_.CheckErrors());
52+
/// ICEBERG_RETURN_UNEXPECTED(CheckErrors());
5853
/// return MyObject{value_};
5954
/// }
55+
///
56+
/// private:
57+
/// int value_ = 0;
6058
/// };
6159
/// \endcode
62-
class ErrorCollector {
60+
class ICEBERG_EXPORT ErrorCollector {
6361
public:
6462
ErrorCollector() = default;
63+
virtual ~ErrorCollector() = default;
6564

66-
/// \brief Add a validation error
65+
ErrorCollector(ErrorCollector&&) = default;
66+
ErrorCollector& operator=(ErrorCollector&&) = default;
67+
68+
ErrorCollector(const ErrorCollector&) = default;
69+
ErrorCollector& operator=(const ErrorCollector&) = default;
70+
71+
/// \brief Add a specific error and return reference to derived class
6772
///
73+
/// \param self Deduced reference to the derived class instance
6874
/// \param kind The kind of error
6975
/// \param message The error message
70-
void AddError(ErrorKind kind, std::string message) {
71-
errors_.emplace_back(kind, std::move(message));
76+
/// \return Reference to the derived class for method chaining
77+
auto& AddError(this auto& self, ErrorKind kind, std::string message) {
78+
self.errors_.emplace_back(kind, std::move(message));
79+
return self;
7280
}
7381

74-
/// \brief Add an existing error object
82+
/// \brief Add an existing error object and return reference to derived class
7583
///
7684
/// Useful when propagating errors from other components or reusing
7785
/// error objects without deconstructing and reconstructing them.
7886
///
87+
/// \param self Deduced reference to the derived class instance
7988
/// \param err The error to add
80-
void AddError(Error err) { errors_.push_back(std::move(err)); }
89+
/// \return Reference to the derived class for method chaining
90+
auto& AddError(this auto& self, Error err) {
91+
self.errors_.push_back(std::move(err));
92+
return self;
93+
}
8194

8295
/// \brief Check if any errors have been collected
8396
///
8497
/// \return true if there are accumulated errors
85-
bool HasErrors() const { return !errors_.empty(); }
98+
[[nodiscard]] bool HasErrors() const { return !errors_.empty(); }
8699

87100
/// \brief Get the number of errors collected
88101
///
89102
/// \return The count of accumulated errors
90-
size_t ErrorCount() const { return errors_.size(); }
103+
[[nodiscard]] size_t ErrorCount() const { return errors_.size(); }
91104

92105
/// \brief Check for accumulated errors and return them if any exist
93106
///
@@ -97,7 +110,7 @@ class ErrorCollector {
97110
///
98111
/// \return Status::OK if no errors, or a ValidationFailed error with
99112
/// all accumulated error messages
100-
Status CheckErrors() const {
113+
[[nodiscard]] Status CheckErrors() const {
101114
if (!errors_.empty()) {
102115
std::string error_msg = "Validation failed due to the following errors:\n";
103116
for (const auto& [kind, message] : errors_) {
@@ -117,9 +130,9 @@ class ErrorCollector {
117130
/// \brief Get read-only access to all collected errors
118131
///
119132
/// \return A const reference to the vector of errors
120-
const std::vector<Error>& Errors() const { return errors_; }
133+
[[nodiscard]] const std::vector<Error>& Errors() const { return errors_; }
121134

122-
private:
135+
protected:
123136
std::vector<Error> errors_;
124137
};
125138

src/iceberg/util/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ install_headers(
2323
'conversions.h',
2424
'decimal.h',
2525
'endian.h',
26+
'error_collector.h',
2627
'formattable.h',
2728
'formatter.h',
2829
'int128.h',

0 commit comments

Comments
 (0)