From c491a7f21166be67309d6d9816c9d0c12ec979e6 Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Thu, 20 Nov 2025 09:05:17 -0800 Subject: [PATCH 1/6] feat: add PendingUpdate interface for table changes 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. --- src/iceberg/pending_update.h | 75 ++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 src/iceberg/pending_update.h diff --git a/src/iceberg/pending_update.h b/src/iceberg/pending_update.h new file mode 100644 index 000000000..3658b43b3 --- /dev/null +++ b/src/iceberg/pending_update.h @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/pending_update.h +/// API for table changes using builder pattern + +#include "iceberg/iceberg_export.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" + +namespace iceberg { + +/// \brief Base class for table metadata changes using builder pattern +/// +/// PendingUpdate represents an API for building and committing changes to an +/// Iceberg table. Subclasses implement specific types of table updates such as +/// schema changes, property updates, or snapshot-producing operations like +/// appends and deletes. +/// +/// Apply() can be used to validate and inspect the uncommitted changes before +/// committing. Commit() applies the changes and commits them to the table. +/// +/// \tparam T The type of result returned by Apply() +template +class ICEBERG_EXPORT PendingUpdate { + public: + virtual ~PendingUpdate() = default; + + /// \brief Apply the pending changes and return the uncommitted result + /// + /// This does not result in a permanent update. + /// + /// \return the uncommitted changes that would be committed, or an error if + /// the pending changes cannot be applied to the current table metadata + virtual Result Apply() = 0; + + /// \brief Apply and commit the pending changes to the table + /// + /// Changes are committed by calling the underlying table's commit operation. + /// + /// Once the commit is successful, the updated table will be refreshed. + /// + /// \return Status::OK if the commit was successful, or an error status if + /// validation failed or the commit encountered conflicts + virtual Status Commit() = 0; + + protected: + PendingUpdate() = default; + + // Non-copyable, movable + PendingUpdate(const PendingUpdate&) = delete; + PendingUpdate& operator=(const PendingUpdate&) = delete; + PendingUpdate(PendingUpdate&&) noexcept = default; + PendingUpdate& operator=(PendingUpdate&&) noexcept = default; +}; + +} // namespace iceberg From 9f8b0dac1380ad28cd902afd237896e94c8e8d86 Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Thu, 20 Nov 2025 11:56:02 -0800 Subject: [PATCH 2/6] fix: move deleted copy/assignment operators to public section Deleted member functions should be public for better error messages and to follow C++ best practices. Matches the pattern used in TableMetadataBuilder. --- src/iceberg/pending_update.h | 69 +++++++++++++++++++++++------------- src/iceberg/result.h | 2 ++ src/iceberg/type_fwd.h | 4 +++ 3 files changed, 50 insertions(+), 25 deletions(-) diff --git a/src/iceberg/pending_update.h b/src/iceberg/pending_update.h index 3658b43b3..d89413a89 100644 --- a/src/iceberg/pending_update.h +++ b/src/iceberg/pending_update.h @@ -28,48 +28,67 @@ namespace iceberg { -/// \brief Base class for table metadata changes using builder pattern +/// \brief Non-template base class for table metadata changes /// -/// PendingUpdate represents an API for building and committing changes to an -/// Iceberg table. Subclasses implement specific types of table updates such as -/// schema changes, property updates, or snapshot-producing operations like -/// appends and deletes. +/// This base class allows storing different types of PendingUpdate operations +/// in the same collection (e.g., in Transaction). It provides the common Commit() +/// interface that all updates share. +/// +/// This matches the Java Iceberg pattern where BaseTransaction stores a +/// List without type parameters. +class ICEBERG_EXPORT PendingUpdateBase { + public: + virtual ~PendingUpdateBase() = default; + + /// \brief Apply and commit the pending changes to the table + /// + /// Changes are committed by calling the underlying table's commit operation. + /// + /// Once the commit is successful, the updated table will be refreshed. + /// + /// \return Status::OK if the commit was successful, or an error: + /// - ValidationFailed: if update cannot be applied to current metadata + /// - CommitFailed: if update cannot be committed due to conflicts + /// - CommitStateUnknown: if commit success state is unknown + virtual Status Commit() = 0; + + // Non-copyable, movable + PendingUpdateBase(const PendingUpdateBase&) = delete; + PendingUpdateBase& operator=(const PendingUpdateBase&) = delete; + PendingUpdateBase(PendingUpdateBase&&) noexcept = default; + PendingUpdateBase& operator=(PendingUpdateBase&&) noexcept = default; + + protected: + PendingUpdateBase() = default; +}; + +/// \brief Template class for type-safe table metadata changes +/// +/// PendingUpdate extends PendingUpdateBase with a type-safe Apply() method that +/// returns the specific result type for each operation. Subclasses implement +/// specific types of table updates such as schema changes, property updates, or +/// snapshot-producing operations like appends and deletes. /// /// Apply() can be used to validate and inspect the uncommitted changes before /// committing. Commit() applies the changes and commits them to the table. /// /// \tparam T The type of result returned by Apply() template -class ICEBERG_EXPORT PendingUpdate { +class ICEBERG_EXPORT PendingUpdate : public PendingUpdateBase { public: - virtual ~PendingUpdate() = default; + ~PendingUpdate() override = default; /// \brief Apply the pending changes and return the uncommitted result /// /// This does not result in a permanent update. /// - /// \return the uncommitted changes that would be committed, or an error if - /// the pending changes cannot be applied to the current table metadata + /// \return the uncommitted changes that would be committed, or an error: + /// - ValidationFailed: if pending changes cannot be applied + /// - InvalidArgument: if pending changes are conflicting virtual Result Apply() = 0; - /// \brief Apply and commit the pending changes to the table - /// - /// Changes are committed by calling the underlying table's commit operation. - /// - /// Once the commit is successful, the updated table will be refreshed. - /// - /// \return Status::OK if the commit was successful, or an error status if - /// validation failed or the commit encountered conflicts - virtual Status Commit() = 0; - protected: PendingUpdate() = default; - - // Non-copyable, movable - PendingUpdate(const PendingUpdate&) = delete; - PendingUpdate& operator=(const PendingUpdate&) = delete; - PendingUpdate(PendingUpdate&&) noexcept = default; - PendingUpdate& operator=(PendingUpdate&&) noexcept = default; }; } // namespace iceberg diff --git a/src/iceberg/result.h b/src/iceberg/result.h index 99df37247..fe7924e3e 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -35,6 +35,7 @@ enum class ErrorKind { kDecompressError, kInvalid, // For general invalid errors kInvalidArgument, + kValidationFailed, kInvalidArrowData, kInvalidExpression, kInvalidSchema, @@ -84,6 +85,7 @@ DEFINE_ERROR_FUNCTION(CommitStateUnknown) DEFINE_ERROR_FUNCTION(DecompressError) DEFINE_ERROR_FUNCTION(Invalid) DEFINE_ERROR_FUNCTION(InvalidArgument) +DEFINE_ERROR_FUNCTION(ValidationFailed) DEFINE_ERROR_FUNCTION(InvalidArrowData) DEFINE_ERROR_FUNCTION(InvalidExpression) DEFINE_ERROR_FUNCTION(InvalidSchema) diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 79b43f5fd..3c40d9253 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -154,6 +154,10 @@ class TableRequirement; class TableMetadataBuilder; class TableUpdateContext; +class PendingUpdateBase; +template +class PendingUpdate; + /// ---------------------------------------------------------------------------- /// TODO: Forward declarations below are not added yet. /// ---------------------------------------------------------------------------- From 04539fda029a2c1c8ee95ca627f761cab597d23d Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Sun, 23 Nov 2025 20:34:33 -0800 Subject: [PATCH 3/6] Add unit tests for PendingUpdate interface 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. --- src/iceberg/test/CMakeLists.txt | 3 +- src/iceberg/test/pending_update_test.cc | 99 +++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 src/iceberg/test/pending_update_test.cc diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index d82fe17b8..ca20c0be6 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -83,7 +83,8 @@ add_iceberg_test(table_test json_internal_test.cc table_test.cc schema_json_test.cc - table_metadata_builder_test.cc) + table_metadata_builder_test.cc + pending_update_test.cc) add_iceberg_test(expression_test SOURCES diff --git a/src/iceberg/test/pending_update_test.cc b/src/iceberg/test/pending_update_test.cc new file mode 100644 index 000000000..b9fa1b94c --- /dev/null +++ b/src/iceberg/test/pending_update_test.cc @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/pending_update.h" + +#include + +#include "iceberg/result.h" +#include "iceberg/test/matchers.h" + +namespace iceberg { + +// Mock implementation for testing the interface +class MockSnapshot {}; + +class MockPendingUpdate : public PendingUpdate { + public: + MockPendingUpdate() = default; + + Result Apply() override { + if (should_fail_) { + return ValidationFailed("Mock validation failed"); + } + return MockSnapshot{}; + } + + Status Commit() override { + if (should_fail_commit_) { + return CommitFailed("Mock commit failed"); + } + apply_called_ = true; + commit_called_ = true; + return {}; + } + + void SetShouldFail(bool fail) { should_fail_ = fail; } + void SetShouldFailCommit(bool fail) { should_fail_commit_ = fail; } + bool ApplyCalled() const { return apply_called_; } + bool CommitCalled() const { return commit_called_; } + + private: + bool should_fail_ = false; + bool should_fail_commit_ = false; + bool apply_called_ = false; + bool commit_called_ = false; +}; + +TEST(PendingUpdateTest, ApplySuccess) { + MockPendingUpdate update; + auto result = update.Apply(); + EXPECT_THAT(result, IsOk()); +} + +TEST(PendingUpdateTest, ApplyValidationFailed) { + MockPendingUpdate update; + update.SetShouldFail(true); + auto result = update.Apply(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Mock validation failed")); +} + +TEST(PendingUpdateTest, CommitSuccess) { + MockPendingUpdate update; + auto status = update.Commit(); + EXPECT_THAT(status, IsOk()); + EXPECT_TRUE(update.CommitCalled()); +} + +TEST(PendingUpdateTest, CommitFailed) { + MockPendingUpdate update; + update.SetShouldFailCommit(true); + auto status = update.Commit(); + EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed)); + EXPECT_THAT(status, HasErrorMessage("Mock commit failed")); +} + +TEST(PendingUpdateTest, BaseClassPolymorphism) { + std::unique_ptr base_ptr = std::make_unique(); + auto status = base_ptr->Commit(); + EXPECT_THAT(status, IsOk()); +} + +} // namespace iceberg From 649f539cc0449857b8f5e58397683b1383c5526f Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Sun, 23 Nov 2025 21:06:55 -0800 Subject: [PATCH 4/6] docs: restore builder pattern context in PendingUpdate documentation The builder pattern context explains why constructors are protected - subclasses should be created through their builder methods, not directly instantiated. --- src/iceberg/pending_update.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/iceberg/pending_update.h b/src/iceberg/pending_update.h index d89413a89..63a4f5329 100644 --- a/src/iceberg/pending_update.h +++ b/src/iceberg/pending_update.h @@ -28,7 +28,7 @@ namespace iceberg { -/// \brief Non-template base class for table metadata changes +/// \brief Non-template base class for table metadata changes using builder pattern /// /// This base class allows storing different types of PendingUpdate operations /// in the same collection (e.g., in Transaction). It provides the common Commit() @@ -62,7 +62,7 @@ class ICEBERG_EXPORT PendingUpdateBase { PendingUpdateBase() = default; }; -/// \brief Template class for type-safe table metadata changes +/// \brief Template class for type-safe table metadata changes using builder pattern /// /// PendingUpdate extends PendingUpdateBase with a type-safe Apply() method that /// returns the specific result type for each operation. Subclasses implement From 4d560bd9736abea2d6b4d37a83db795f4488404d Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Mon, 24 Nov 2025 07:47:55 -0800 Subject: [PATCH 5/6] refactor: rename PendingUpdateBase to PendingUpdate for cleaner API Renamed: - PendingUpdateBase -> PendingUpdate (non-template base class) - PendingUpdate -> PendingUpdateTyped (template class) This makes the API more intuitive: std::vector> is cleaner than std::vector> --- src/iceberg/pending_update.h | 24 ++++++++++++------------ src/iceberg/test/pending_update_test.cc | 4 ++-- src/iceberg/type_fwd.h | 4 ++-- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/iceberg/pending_update.h b/src/iceberg/pending_update.h index 63a4f5329..4c6fe095f 100644 --- a/src/iceberg/pending_update.h +++ b/src/iceberg/pending_update.h @@ -28,7 +28,7 @@ namespace iceberg { -/// \brief Non-template base class for table metadata changes using builder pattern +/// \brief Base class for table metadata changes using builder pattern /// /// This base class allows storing different types of PendingUpdate operations /// in the same collection (e.g., in Transaction). It provides the common Commit() @@ -36,9 +36,9 @@ namespace iceberg { /// /// This matches the Java Iceberg pattern where BaseTransaction stores a /// List without type parameters. -class ICEBERG_EXPORT PendingUpdateBase { +class ICEBERG_EXPORT PendingUpdate { public: - virtual ~PendingUpdateBase() = default; + virtual ~PendingUpdate() = default; /// \brief Apply and commit the pending changes to the table /// @@ -53,18 +53,18 @@ class ICEBERG_EXPORT PendingUpdateBase { virtual Status Commit() = 0; // Non-copyable, movable - PendingUpdateBase(const PendingUpdateBase&) = delete; - PendingUpdateBase& operator=(const PendingUpdateBase&) = delete; - PendingUpdateBase(PendingUpdateBase&&) noexcept = default; - PendingUpdateBase& operator=(PendingUpdateBase&&) noexcept = default; + PendingUpdate(const PendingUpdate&) = delete; + PendingUpdate& operator=(const PendingUpdate&) = delete; + PendingUpdate(PendingUpdate&&) noexcept = default; + PendingUpdate& operator=(PendingUpdate&&) noexcept = default; protected: - PendingUpdateBase() = default; + PendingUpdate() = default; }; /// \brief Template class for type-safe table metadata changes using builder pattern /// -/// PendingUpdate extends PendingUpdateBase with a type-safe Apply() method that +/// PendingUpdateTyped extends PendingUpdate with a type-safe Apply() method that /// returns the specific result type for each operation. Subclasses implement /// specific types of table updates such as schema changes, property updates, or /// snapshot-producing operations like appends and deletes. @@ -74,9 +74,9 @@ class ICEBERG_EXPORT PendingUpdateBase { /// /// \tparam T The type of result returned by Apply() template -class ICEBERG_EXPORT PendingUpdate : public PendingUpdateBase { +class ICEBERG_EXPORT PendingUpdateTyped : public PendingUpdate { public: - ~PendingUpdate() override = default; + ~PendingUpdateTyped() override = default; /// \brief Apply the pending changes and return the uncommitted result /// @@ -88,7 +88,7 @@ class ICEBERG_EXPORT PendingUpdate : public PendingUpdateBase { virtual Result Apply() = 0; protected: - PendingUpdate() = default; + PendingUpdateTyped() = default; }; } // namespace iceberg diff --git a/src/iceberg/test/pending_update_test.cc b/src/iceberg/test/pending_update_test.cc index b9fa1b94c..35974b36f 100644 --- a/src/iceberg/test/pending_update_test.cc +++ b/src/iceberg/test/pending_update_test.cc @@ -29,7 +29,7 @@ namespace iceberg { // Mock implementation for testing the interface class MockSnapshot {}; -class MockPendingUpdate : public PendingUpdate { +class MockPendingUpdate : public PendingUpdateTyped { public: MockPendingUpdate() = default; @@ -91,7 +91,7 @@ TEST(PendingUpdateTest, CommitFailed) { } TEST(PendingUpdateTest, BaseClassPolymorphism) { - std::unique_ptr base_ptr = std::make_unique(); + std::unique_ptr base_ptr = std::make_unique(); auto status = base_ptr->Commit(); EXPECT_THAT(status, IsOk()); } diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 3c40d9253..363f4c0be 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -154,9 +154,9 @@ class TableRequirement; class TableMetadataBuilder; class TableUpdateContext; -class PendingUpdateBase; -template class PendingUpdate; +template +class PendingUpdateTyped; /// ---------------------------------------------------------------------------- /// TODO: Forward declarations below are not added yet. From 8187000272068aa6a800fbff814a6740a3f98020 Mon Sep 17 00:00:00 2001 From: Xinli Shang Date: Tue, 25 Nov 2025 09:58:02 -0800 Subject: [PATCH 6/6] refactor: sort ErrorKind enum and error functions alphabetically - 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() --- src/iceberg/result.h | 8 ++++---- src/iceberg/test/CMakeLists.txt | 6 +++--- src/iceberg/test/pending_update_test.cc | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/iceberg/result.h b/src/iceberg/result.h index fe7924e3e..743473b10 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -35,12 +35,11 @@ enum class ErrorKind { kDecompressError, kInvalid, // For general invalid errors kInvalidArgument, - kValidationFailed, kInvalidArrowData, kInvalidExpression, - kInvalidSchema, kInvalidManifest, kInvalidManifestList, + kInvalidSchema, kIOError, kJsonParseError, kNoSuchNamespace, @@ -50,6 +49,7 @@ enum class ErrorKind { kNotImplemented, kNotSupported, kUnknownError, + kValidationFailed, }; /// \brief Error with a kind and a message. @@ -85,12 +85,11 @@ DEFINE_ERROR_FUNCTION(CommitStateUnknown) DEFINE_ERROR_FUNCTION(DecompressError) DEFINE_ERROR_FUNCTION(Invalid) DEFINE_ERROR_FUNCTION(InvalidArgument) -DEFINE_ERROR_FUNCTION(ValidationFailed) DEFINE_ERROR_FUNCTION(InvalidArrowData) DEFINE_ERROR_FUNCTION(InvalidExpression) -DEFINE_ERROR_FUNCTION(InvalidSchema) DEFINE_ERROR_FUNCTION(InvalidManifest) DEFINE_ERROR_FUNCTION(InvalidManifestList) +DEFINE_ERROR_FUNCTION(InvalidSchema) DEFINE_ERROR_FUNCTION(IOError) DEFINE_ERROR_FUNCTION(JsonParseError) DEFINE_ERROR_FUNCTION(NoSuchNamespace) @@ -100,6 +99,7 @@ DEFINE_ERROR_FUNCTION(NotFound) DEFINE_ERROR_FUNCTION(NotImplemented) DEFINE_ERROR_FUNCTION(NotSupported) DEFINE_ERROR_FUNCTION(UnknownError) +DEFINE_ERROR_FUNCTION(ValidationFailed) #undef DEFINE_ERROR_FUNCTION diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index ca20c0be6..1c6cf7cfe 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -79,12 +79,12 @@ add_iceberg_test(schema_test add_iceberg_test(table_test SOURCES - test_common.cc json_internal_test.cc - table_test.cc + pending_update_test.cc schema_json_test.cc table_metadata_builder_test.cc - pending_update_test.cc) + table_test.cc + test_common.cc) add_iceberg_test(expression_test SOURCES diff --git a/src/iceberg/test/pending_update_test.cc b/src/iceberg/test/pending_update_test.cc index 35974b36f..bc26304f7 100644 --- a/src/iceberg/test/pending_update_test.cc +++ b/src/iceberg/test/pending_update_test.cc @@ -37,6 +37,7 @@ class MockPendingUpdate : public PendingUpdateTyped { if (should_fail_) { return ValidationFailed("Mock validation failed"); } + apply_called_ = true; return MockSnapshot{}; } @@ -44,7 +45,6 @@ class MockPendingUpdate : public PendingUpdateTyped { if (should_fail_commit_) { return CommitFailed("Mock commit failed"); } - apply_called_ = true; commit_called_ = true; return {}; }