feat: add PendingUpdate interface for table changes #334

shangxinli · 2025-11-20T17:09:35Z

Add PendingUpdate template class that provides a builder pattern API for constructing and committing table metadata changes. This interface follows the Java PendingUpdate design with C++ idioms:

Apply() returns Result with uncommitted changes for validation
Commit() returns Status and atomically commits changes to the table
Template parameter T allows type-safe results (Snapshot, Schema, etc.)

The interface is designed for future implementations like AppendFiles, UpdateSchema, UpdateProperties, and other table update operations.

Add PendingUpdate<T> template class that provides a builder pattern API for constructing and committing table metadata changes. This interface follows the Java PendingUpdate design with C++ idioms: - Apply() returns Result<T> with uncommitted changes for validation - Commit() returns Status and atomically commits changes to the table - Template parameter T allows type-safe results (Snapshot, Schema, etc.) The interface is designed for future implementations like AppendFiles, UpdateSchema, UpdateProperties, and other table update operations.

gty404 · 2025-11-21T04:15:25Z

src/iceberg/pending_update.h

+  ///
+  /// \return Status::OK if the commit was successful, or an error status if
+  ///         validation failed or the commit encountered conflicts
+  virtual Status Commit() = 0;


Define explicit Status codes for commit failure categories (validation failed, conflicts, commit-state-unknown) so callers can distinguish the same cases Java documents ?

I think we have already added some. If not, we can extend it: https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/result.h#L31. Is this what you expected?

My meaning is that the status code returned by the Commit interface is in what scenario, and if the status code is clearly named, it can follow the definition in result.h.

gty404 · 2025-11-21T04:29:47Z

src/iceberg/pending_update.h

+///
+/// \tparam T The type of result returned by Apply()
+template <typename T>
+class ICEBERG_EXPORT PendingUpdate {


Both PendingUpdateBase/PendingUpdateTyped<T> and PendingUpdate<T> have advantages and disadvantages for the caller. Does iceberg-java have scenarios where PendingUpdate is used directly?

If we want to use the same approach in the Transaction, we need to make PendingUpdate non-templated.

Yes, if we need to collect a list of PendingUpdate, we need PendingUpdate without template parameters. We can obtain the specific implementation class through type promotion.

Yeah, make sense

Deleted member functions should be public for better error messages and to follow C++ best practices. Matches the pattern used in TableMetadataBuilder.

Jinchul81

Please don't forget to add unit tests for PendingUpdate in the follow up PRs.

Jinchul81 · 2025-11-22T18:37:36Z

src/iceberg/pending_update.h

+  PendingUpdate() = default;
+
+  // Non-copyable, movable
+  PendingUpdate(const PendingUpdate&) = delete;


Please move it to the public space.

Jinchul81 · 2025-11-22T18:37:44Z

src/iceberg/pending_update.h

+
+  // Non-copyable, movable
+  PendingUpdate(const PendingUpdate&) = delete;
+  PendingUpdate& operator=(const PendingUpdate&) = delete;


Please move it to the public space.

Jinchul81 · 2025-11-22T18:43:18Z

src/iceberg/pending_update.h


 namespace iceberg {

-/// \brief Base class for table metadata changes using builder pattern


I don't prefer you to delete the context that you're using builder pattern. This is the reason why the ctors of this class are in the protected region, right?

shangxinli · 2025-11-24T05:03:59Z

Please don't forget to add unit tests for PendingUpdate in the follow up PRs.

Just added

wgtmac · 2025-11-24T14:13:47Z

src/iceberg/pending_update.h

+///
+/// This matches the Java Iceberg pattern where BaseTransaction stores a
+/// List<PendingUpdate> without type parameters.
+class ICEBERG_EXPORT PendingUpdateBase {


Suggested change

class ICEBERG_EXPORT PendingUpdateBase {

class ICEBERG_EXPORT PendingUpdate {

Should we use PendingUpdate as the base name? std::vector<std::unique_ptr<PendingUpdate>> looks nicer than std::vector<std::unique_ptr<PendingUpdateBase>> IMHO.

wgtmac · 2025-11-24T14:14:12Z

src/iceberg/pending_update.h

+///
+/// \tparam T The type of result returned by Apply()
+template <typename T>
+class ICEBERG_EXPORT PendingUpdate : public PendingUpdateBase {


We can rename it to BasePendingUpdate.

Rename it ti PendingUpdateTyped

Added comprehensive unit tests for the PendingUpdate interface including: - Basic Apply() success and validation failure scenarios - Commit() success and failure scenarios - Base class polymorphism to verify PendingUpdateBase works correctly These tests verify the interface compiles correctly and can be properly implemented by subclasses.

The builder pattern context explains why constructors are protected - subclasses should be created through their builder methods, not directly instantiated.

Renamed: - PendingUpdateBase -> PendingUpdate (non-template base class) - PendingUpdate<T> -> PendingUpdateTyped<T> (template class) This makes the API more intuitive: std::vector<std::unique_ptr<PendingUpdate>> is cleaner than std::vector<std::unique_ptr<PendingUpdateBase>>

wgtmac

Generally LGTM. I've left some minor comments. Thanks @shangxinli!

@gty404 @Jinchul81 Do you have more comments?

wgtmac · 2025-11-25T04:45:40Z

src/iceberg/result.h

  kDecompressError,
  kInvalid,  // For general invalid errors
  kInvalidArgument,
+  kValidationFailed,


Please sort it alphabetically.

wgtmac · 2025-11-25T04:45:52Z

src/iceberg/result.h

 DEFINE_ERROR_FUNCTION(DecompressError)
 DEFINE_ERROR_FUNCTION(Invalid)
 DEFINE_ERROR_FUNCTION(InvalidArgument)
+DEFINE_ERROR_FUNCTION(ValidationFailed)


wgtmac · 2025-11-25T04:46:29Z

src/iceberg/test/CMakeLists.txt

                 schema_json_test.cc
-                 table_metadata_builder_test.cc)
+                 table_metadata_builder_test.cc
+                 pending_update_test.cc)


These files are unfortunately not sorted alphabetically. Could you help fix it as well?

HeartLinked

LGTM. I only have a minor question and comment.

HeartLinked · 2025-11-25T10:44:22Z

src/iceberg/test/pending_update_test.cc

+    if (should_fail_commit_) {
+      return CommitFailed("Mock commit failed");
+    }
+    apply_called_ = true;


Should we remove apply_called_ = true here? I don't think we should assume that Apply has already been called by the user when Commit is invoked.I haven't seen any content in the comments that requires users to guarantee this.

- Move kValidationFailed to the end of ErrorKind enum after kUnknownError - Reorder kInvalidSchema to come after kInvalidManifestList - Sort DEFINE_ERROR_FUNCTION calls to match enum order - Sort table_test source files alphabetically in CMakeLists.txt - Fix apply_called_ tracking to be set in Apply() method instead of Commit()

gty404 reviewed Nov 21, 2025

View reviewed changes

shangxinli force-pushed the pending_update branch from bb42e85 to b840e7a Compare November 22, 2025 01:32

fix: move deleted copy/assignment operators to public section

9f8b0da

Deleted member functions should be public for better error messages and to follow C++ best practices. Matches the pattern used in TableMetadataBuilder.

shangxinli force-pushed the pending_update branch from 3c74603 to 9f8b0da Compare November 22, 2025 01:53

Jinchul81 reviewed Nov 22, 2025

View reviewed changes

shangxinli force-pushed the pending_update branch from 50d119f to 55044b3 Compare November 24, 2025 05:09

wgtmac reviewed Nov 24, 2025

View reviewed changes

shangxinli force-pushed the pending_update branch 2 times, most recently from e1de414 to 4852fcb Compare November 24, 2025 15:48

shangxinli added 3 commits November 24, 2025 07:51

docs: restore builder pattern context in PendingUpdate documentation

649f539

The builder pattern context explains why constructors are protected - subclasses should be created through their builder methods, not directly instantiated.

shangxinli force-pushed the pending_update branch from 4852fcb to 4d560bd Compare November 24, 2025 15:52

wgtmac approved these changes Nov 25, 2025

View reviewed changes

HeartLinked approved these changes Nov 25, 2025

View reviewed changes

gty404 approved these changes Nov 26, 2025

View reviewed changes

wgtmac approved these changes Nov 26, 2025

View reviewed changes

wgtmac merged commit 2adb461 into apache:main Nov 26, 2025
10 checks passed

shangxinli deleted the pending_update branch November 26, 2025 16:01


		namespace iceberg {

		/// \brief Base class for table metadata changes using builder pattern

	class ICEBERG_EXPORT PendingUpdateBase {
	class ICEBERG_EXPORT PendingUpdate {

feat: add PendingUpdate interface for table changes #334

feat: add PendingUpdate interface for table changes #334

Uh oh!

Conversation

shangxinli commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jinchul81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangxinli commented Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartLinked left a comment

Choose a reason for hiding this comment

Uh oh!

HeartLinked Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HeartLinked Nov 25, 2025 •

edited

Loading