keep a single API shape for ErrorCollector and SnapshotUpdate

Author: PingLiuPing

src/iceberg/update/snapshot_update.h

@@ -62,112 +62,10 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
   bool IsRetryable() const override { return true; }
   Status Commit() override;
 
-// C++23 uses deducing `this` so chaining keeps returning the derived update type.
-// C++20 exposes the same operations but returns SnapshotUpdate&, because preserving
-// the derived type would require making the update hierarchy CRTP. Keyed on the
-// language version rather than __cpp_explicit_this_parameter, which Apple clang does
-// not define, and compared with "> 202002L" because MSVC's /std:c++latest only
-// promises a value above 202002L.
-#if __cplusplus > 202002L || (defined(_MSVC_LANG) && _MSVC_LANG > 202002L)
   /// \brief Set the metrics reporter for this snapshot update.
   ///
   /// \param reporter The metrics reporter to use.
   /// \return Reference to this for method chaining.
-  auto& ReportWith(this auto& self, std::shared_ptr<MetricsReporter> reporter) {
-    static_cast<SnapshotUpdate&>(self).reporter_ = std::move(reporter);
-    return self;
-  }
-
-  /// \brief Set a callback to delete files instead of the table's default.
-  ///
-  /// \param delete_func A function used to delete file locations.
-  /// \return This update for method chaining.
-  /// \note Cannot be called more than once.
-  auto& DeleteWith(this auto& self,
-                   std::function<Status(const std::string&)> delete_func) {
-    if (self.delete_func_) {
-      return self.AddError(ErrorKind::kInvalidArgument,
-                           "Cannot set delete callback more than once");
-    }
-    self.delete_func_ = std::move(delete_func);
-    return self;
-  }
-
-  /// \brief Stage a snapshot in table metadata, but do not make it current.
-  ///
-  /// The snapshot is assigned an ID and added to table metadata. The table's
-  /// current snapshot ID is not updated.
-  ///
-  /// \return This update for method chaining.
-  auto& StageOnly(this auto& self) {
-    self.stage_only_ = true;
-    return self;
-  }
-
-  /// \brief Configure an executor for manifest planning work.
-  ///
-  /// \param executor Executor to use while planning manifests.
-  /// \return Reference to this for method chaining.
-  auto& ScanManifestsWith(this auto& self, Executor& executor) {
-    self.plan_executor_ = std::ref(executor);
-    return self;
-  }
-
-  /// \brief Perform operations on a particular branch.
-  ///
-  /// \param branch The name of a SnapshotRef of type branch.
-  /// \return This update for method chaining.
-  auto& ToBranch(this auto& self, const std::string& branch) {
-    if (branch.empty()) [[unlikely]] {
-      return self.AddError(ErrorKind::kInvalidArgument, "Branch name cannot be empty");
-    }
-
-    if (auto ref_it = self.base().refs.find(branch); ref_it != self.base().refs.end()) {
-      if (ref_it->second->type() != SnapshotRefType::kBranch) {
-        return self.AddError(ErrorKind::kInvalidArgument,
-                             "{} is a tag, not a branch. Tags cannot be targets for "
-                             "producing snapshots",
-                             branch);
-      }
-    }
-
-    self.target_branch_ = branch;
-    return self;
-  }
-
-  /// \brief Set a summary property in the snapshot produced by this update.
-  ///
-  /// \param property A String property name.
-  /// \param value A String property value.
-  /// \return This update for method chaining.
-  auto& Set(this auto& self, const std::string& property, const std::string& value) {
-    static_cast<SnapshotUpdate&>(self).SetSummaryProperty(property, value);
-    return self;
-  }
-
-  /// \brief Configure an executor and max writer count for writing new manifests.
-  ///
-  /// If this method is not called, manifest writes remain serial. When configured,
-  /// files may be split into independent rolling-writer groups.
-  ///
-  /// \note Custom FileIO implementations and registered writer factories used for
-  /// manifest writes must support concurrent calls when an executor is configured.
-  auto& WriteManifestsWith(this auto& self, Executor& executor, int32_t parallelism) {
-    if (parallelism <= 0) [[unlikely]] {
-      return self.AddError(
-          ErrorKind::kInvalidArgument,
-          "Manifest write parallelism must be greater than 0, but was: {}", parallelism);
-    }
-
-    self.write_manifest_executor_ = std::ref(executor);
-    self.write_manifest_parallelism_ = parallelism;
-    return self;
-  }
-#else
-  /// \brief Set the metrics reporter for this snapshot update.
-  ///
-  /// \param reporter The metrics reporter to use.
-  /// \return This snapshot update for method chaining.
   SnapshotUpdate& ReportWith(std::shared_ptr<MetricsReporter> reporter) {
     reporter_ = std::move(reporter);
     return *this;
@@ -176,7 +74,7 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
   /// \brief Set a callback to delete files instead of the table's default.
   ///
   /// \param delete_func A function used to delete file locations.
-  /// \return This snapshot update for method chaining.
+  /// \return This update for method chaining.
   /// \note Cannot be called more than once.
   SnapshotUpdate& DeleteWith(std::function<Status(const std::string&)> delete_func) {
     if (delete_func_) {
@@ -189,7 +87,10 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
 
   /// \brief Stage a snapshot in table metadata, but do not make it current.
   ///
-  /// \return This snapshot update for method chaining.
+  /// The snapshot is assigned an ID and added to table metadata. The table's
+  /// current snapshot ID is not updated.
+  ///
+  /// \return This update for method chaining.
   SnapshotUpdate& StageOnly() {
     stage_only_ = true;
     return *this;
@@ -198,7 +99,7 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
   /// \brief Configure an executor for manifest planning work.
   ///
   /// \param executor Executor to use while planning manifests.
-  /// \return This snapshot update for method chaining.
+  /// \return Reference to this for method chaining.
   SnapshotUpdate& ScanManifestsWith(Executor& executor) {
     plan_executor_ = std::ref(executor);
     return *this;
@@ -207,7 +108,7 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
   /// \brief Perform operations on a particular branch.
   ///
   /// \param branch The name of a SnapshotRef of type branch.
-  /// \return This snapshot update for method chaining.
+  /// \return This update for method chaining.
   SnapshotUpdate& ToBranch(const std::string& branch) {
     if (branch.empty()) [[unlikely]] {
       AddError(ErrorKind::kInvalidArgument, "Branch name cannot be empty");
@@ -232,17 +133,22 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
   ///
   /// \param property A String property name.
   /// \param value A String property value.
-  /// \return This snapshot update for method chaining.
+  /// \return This update for method chaining.
   SnapshotUpdate& Set(const std::string& property, const std::string& value) {
     SetSummaryProperty(property, value);
     return *this;
   }
 
   /// \brief Configure an executor and max writer count for writing new manifests.
   ///
+  /// If this method is not called, manifest writes remain serial. When configured,
+  /// files may be split into independent rolling-writer groups.
+  ///
   /// \param executor Executor to use while writing manifests.
   /// \param parallelism Maximum number of concurrent manifest writers.
-  /// \return This snapshot update for method chaining.
+  /// \return This update for method chaining.
+  /// \note Custom FileIO implementations and registered writer factories used for
+  /// manifest writes must support concurrent calls when an executor is configured.
   SnapshotUpdate& WriteManifestsWith(Executor& executor, int32_t parallelism) {
     if (parallelism <= 0) [[unlikely]] {
       AddError(ErrorKind::kInvalidArgument,
@@ -255,7 +161,6 @@ class ICEBERG_EXPORT SnapshotUpdate : public PendingUpdate {
     write_manifest_parallelism_ = parallelism;
     return *this;
   }
-#endif  // C++23
 
  protected:
   friend class Transaction;

src/iceberg/update/update_partition_spec.cc

@@ -123,8 +123,8 @@ UpdatePartitionSpec& UpdatePartitionSpec::AddField(const std::shared_ptr<Term>&
     return AddFieldInternal(part_name, source_id, bound_transform->transform());
   }
 
-  return AddError(
-      InvalidArgument("Cannot add {} term to partition spec", term->ToString()));
+  AddError(InvalidArgument("Cannot add {} term to partition spec", term->ToString()));
+  return *this;
 }
 
 UpdatePartitionSpec& UpdatePartitionSpec::AddFieldInternal(
@@ -195,8 +195,9 @@ UpdatePartitionSpec& UpdatePartitionSpec::AddFieldInternal(
         // Rename the old deleted field
         RenameField(existing_field->name(), std::move(renamed));
       } else {
-        return AddError(
+        AddError(
             InvalidArgument("Cannot add duplicate partition field name: {}", field_name));
+        return *this;
       }
     } else {
       // Field is being deleted, rename it to avoid conflict
@@ -257,8 +258,9 @@ UpdatePartitionSpec& UpdatePartitionSpec::RemoveField(const std::shared_ptr<Term
     return RemoveFieldByTransform(key, term->ToString());
   }
 
-  return AddError(
+  AddError(
       InvalidArgument("Cannot remove {} term from partition spec", term->ToString()));
+  return *this;
 }
 
 UpdatePartitionSpec& UpdatePartitionSpec::RemoveFieldByTransform(

src/iceberg/update/update_sort_order.cc

@@ -65,8 +65,9 @@ UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr<Term>& term
     sort_fields_.emplace_back(bound_term->reference()->field_id(),
                               unbound_transform->transform(), direction, null_order);
   } else {
-    return AddError(ErrorKind::kNotSupported, "Not supported unbound term: {}",
-                    static_cast<int>(term->kind()));
+    AddError(ErrorKind::kNotSupported, "Not supported unbound term: {}",
+             static_cast<int>(term->kind()));
+    return *this;
   }
 
   return *this;

src/iceberg/util/error_collector.h

@@ -104,55 +104,6 @@ class ICEBERG_EXPORT ErrorCollector {
   ErrorCollector(const ErrorCollector&) = default;
   ErrorCollector& operator=(const ErrorCollector&) = default;
 
-// C++23 uses deducing `this` so that `return AddError(...)` keeps returning the
-// derived builder type. C++20 exposes the same overloads returning ErrorCollector&,
-// because preserving the derived type would require making the builders CRTP. Keyed
-// on the language version rather than __cpp_explicit_this_parameter, which Apple
-// clang does not define, and compared with "> 202002L" because MSVC's
-// /std:c++latest only promises a value above 202002L.
-#if __cplusplus > 202002L || (defined(_MSVC_LANG) && _MSVC_LANG > 202002L)
-  /// \brief Add a specific error and return reference to derived class
-  ///
-  /// \param self Deduced reference to the derived class instance
-  /// \param kind The kind of error
-  /// \param fmt The format string
-  /// \param args The arguments to format the message
-  /// \return Reference to the derived class for method chaining
-  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;
-  }
-
-  /// \brief Add an existing error object and return reference to derived class
-  ///
-  /// Useful when propagating errors from other components or reusing
-  /// error objects without deconstructing and reconstructing them.
-  ///
-  /// \param self Deduced reference to the derived class instance
-  /// \param err The error to add
-  /// \return Reference to the derived class for method chaining
-  auto& AddError(this auto& self, Error err) {
-    self.errors_.push_back(std::move(err));
-    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, unexpected<Error> err) {
-    self.errors_.push_back(std::move(err.error()));
-    return self;
-  }
-#else
   /// \brief Add a specific error
   ///
   /// \param kind The kind of error
@@ -168,6 +119,9 @@ class ICEBERG_EXPORT ErrorCollector {
 
   /// \brief Add an existing error object
   ///
+  /// Useful when propagating errors from other components or reusing
+  /// error objects without deconstructing and reconstructing them.
+  ///
   /// \param err The error to add
   /// \return This error collector for method chaining
   ErrorCollector& AddError(Error err) {
@@ -177,13 +131,18 @@ class ICEBERG_EXPORT ErrorCollector {
 
   /// \brief Add an unexpected result's error
   ///
+  /// Useful for cases like below:
+  /// \code
+  ///   AddError(InvalidArgument("Invalid value: {}", value));
+  ///   return *this;
+  /// \endcode
+  ///
   /// \param err The unexpected result containing the error to add
   /// \return This error collector for method chaining
   ErrorCollector& AddError(unexpected<Error> err) {
     errors_.push_back(std::move(err.error()));
     return *this;
   }
-#endif  // C++23
 
   /// \brief Check if any errors have been collected
   ///