resolve review comments

gty404 · gty404 · commit 155ae5b6a487 · 2025-10-24T12:28:19.000+08:00
diff --git a/src/iceberg/partition_spec.h b/src/iceberg/partition_spec.h
@@ -47,6 +47,7 @@ class ICEBERG_EXPORT PartitionSpec : public util::Formattable {
   /// \brief The start ID for partition field.  It is only used to generate
   /// partition field id for v1 metadata where it is tracked.
   static constexpr int32_t kLegacyPartitionDataIdStart = 1000;
+  static constexpr int32_t kInvalidPartitionFieldId = -1;
 
   /// \brief Create a new partition spec.
   ///
diff --git a/src/iceberg/schema.h b/src/iceberg/schema.h
@@ -46,6 +46,7 @@ namespace iceberg {
 class ICEBERG_EXPORT Schema : public StructType {
  public:
   static constexpr int32_t kInitialSchemaId = 0;
+  static constexpr int32_t kInvalidColumnId = -1;
 
   explicit Schema(std::vector<SchemaField> fields,
                   std::optional<int32_t> schema_id = std::nullopt);
diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc
@@ -214,7 +214,7 @@ struct TableMetadataBuilder::Impl {
   std::vector<std::unique_ptr<TableUpdate>> changes;
 
   // Error collection (since methods return *this and cannot throw)
-  std::vector<Status> errors;
+  std::vector<Error> errors;
 
   // Metadata location tracking
   std::optional<std::string> metadata_location;
@@ -224,12 +224,12 @@ struct TableMetadataBuilder::Impl {
   explicit Impl(int8_t format_version) : base(nullptr), metadata{} {
     metadata.format_version = format_version;
     metadata.last_sequence_number = TableMetadata::kInitialSequenceNumber;
-    metadata.last_updated_ms = TimePointMs{std::chrono::milliseconds(0)};
-    metadata.last_column_id = 0;
-    metadata.default_spec_id = TableMetadata::kInitialSpecId;
-    metadata.last_partition_id = 0;
-    metadata.current_snapshot_id = TableMetadata::kInvalidSnapshotId;
-    metadata.default_sort_order_id = TableMetadata::kInitialSortOrderId;
+    metadata.last_updated_ms = TimePointMs::min();
+    metadata.last_column_id = Schema::kInvalidColumnId;
+    metadata.default_spec_id = PartitionSpec::kInitialSpecId;
+    metadata.last_partition_id = PartitionSpec::kInvalidPartitionFieldId;
+    metadata.current_snapshot_id = Snapshot::kInvalidSnapshotId;
+    metadata.default_sort_order_id = SortOrder::kInitialSortOrderId;
     metadata.next_row_id = TableMetadata::kInitialRowId;
   }
 
@@ -284,9 +284,9 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID() {
 TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
   std::string uuid_str(uuid);
 
-  // Validation: UUID cannot be null or empty
+  // Validation: UUID cannot be empty
   if (uuid_str.empty()) {
-    impl_->errors.emplace_back(InvalidArgument("Cannot assign null or empty UUID"));
+    impl_->errors.emplace_back(ErrorKind::kInvalidArgument, "Cannot assign empty UUID");
     return *this;
   }
 
@@ -299,7 +299,7 @@ TableMetadataBuilder& TableMetadataBuilder::AssignUUID(std::string_view uuid) {
   impl_->metadata.table_uuid = uuid_str;
 
   // Record the change
-  impl_->changes.push_back(std::make_unique<table::AssignUUID>(uuid_str));
+  impl_->changes.push_back(std::make_unique<table::AssignUUID>(std::move(uuid_str)));
 
   return *this;
 }
@@ -435,42 +435,20 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveEncryptionKey(std::string_view
   throw IcebergError(std::format("{} not implemented", __FUNCTION__));
 }
 
-TableMetadataBuilder& TableMetadataBuilder::DiscardChanges() {
-  // Clear all changes and errors
-  impl_->changes.clear();
-  impl_->errors.clear();
-
-  // Reset metadata to base state
-  if (impl_->base != nullptr) {
-    impl_->metadata = *impl_->base;
-  } else {
-    // Reset to initial state for new table
-    *impl_ = Impl(impl_->metadata.format_version);
-  }
-
-  return *this;
-}
-
 Result<std::unique_ptr<TableMetadata>> TableMetadataBuilder::Build() {
   // 1. Check for accumulated errors
   if (!impl_->errors.empty()) {
     std::string error_msg = "Failed to build TableMetadata due to validation errors:\n";
-    for (const auto& error : impl_->errors) {
-      error_msg += "  - " + error.error().message + "\n";
+    for (const auto& [kind, message] : impl_->errors) {
+      error_msg += "  - " + message + "\n";
     }
     return CommitFailed("{}", error_msg);
   }
 
-  // 2. Validate metadata consistency
-
-  // Validate UUID exists for format version > 1
-  if (impl_->metadata.format_version > 1 && impl_->metadata.table_uuid.empty()) {
-    return InvalidArgument("UUID is required for format version {}",
-                           impl_->metadata.format_version);
-  }
+  // 2. Validate metadata consistency through TableMetadata#Validate
 
   // 3. Update last_updated_ms if there are changes
-  if (!impl_->changes.empty() && impl_->base != nullptr) {
+  if (impl_->metadata.last_updated_ms == TimePointMs::min()) {
     impl_->metadata.last_updated_ms =
         TimePointMs{std::chrono::duration_cast<std::chrono::milliseconds>(
             std::chrono::system_clock::now().time_since_epoch())};
diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h
@@ -73,9 +73,6 @@ struct ICEBERG_EXPORT TableMetadata {
   static constexpr int64_t kInitialSequenceNumber = 0;
   static constexpr int64_t kInvalidSequenceNumber = -1;
   static constexpr int64_t kInitialRowId = 0;
-  static constexpr int32_t kInitialSpecId = 0;
-  static constexpr int32_t kInitialSortOrderId = 1;
-  static constexpr int64_t kInvalidSnapshotId = -1;
 
   /// An integer version number for the format
   int8_t format_version;
@@ -380,13 +377,6 @@ class ICEBERG_EXPORT TableMetadataBuilder {
   /// \return Reference to this builder for method chaining
   TableMetadataBuilder& RemoveEncryptionKey(std::string_view key_id);
 
-  /// \brief Discard all accumulated changes
-  ///
-  /// This is useful when you want to reset the builder state without
-  /// creating a new builder instance.
-  /// \return Reference to this builder for method chaining
-  TableMetadataBuilder& DiscardChanges();
-
   /// \brief Build the TableMetadata object
   ///
   /// \return A Result containing the constructed TableMetadata or an error
diff --git a/src/iceberg/table_requirement.cc b/src/iceberg/table_requirement.cc
@@ -20,7 +20,7 @@
 #include "iceberg/table_requirement.h"
 
 #include "iceberg/table_metadata.h"
-#include "util/string_util.h"
+#include "iceberg/util/string_util.h"
 
 namespace iceberg::table {
 
diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt
@@ -82,9 +82,8 @@ add_iceberg_test(table_test
                  test_common.cc
                  json_internal_test.cc
                  table_test.cc
-                 schema_json_test.cc)
-
-add_iceberg_test(table_metadata_builder_test SOURCES table_metadata_builder_test.cc)
+                 schema_json_test.cc
+                 table_metadata_builder_test.cc)
 
 add_iceberg_test(expression_test
                  SOURCES
diff --git a/src/iceberg/test/matchers.h b/src/iceberg/test/matchers.h
@@ -23,6 +23,7 @@
 #include <gtest/gtest.h>
 
 #include "iceberg/result.h"
+#include "iceberg/util/macros.h"
 
 /*
  * \brief Define custom matchers for expected<T, Error> values
@@ -210,4 +211,17 @@ auto ErrorIs(MatcherT&& matcher) {
       ResultMatcher<std::decay_t<MatcherT>>(false, std::forward<MatcherT>(matcher)));
 }
 
+// Evaluate `rexpr` which should return a Result<T, Error>.
+// On success: assign the contained value to `lhs`.
+// On failure: fail the test with the error message.
+#define ICEBERG_UNWRAP_OR_FAIL_IMPL(result_name, lhs, rexpr)  \
+  auto&& result_name = (rexpr);                               \
+  ASSERT_TRUE(result_name.has_value())                        \
+      << "Operation failed: " << result_name.error().message; \
+  lhs = std::move(result_name.value());
+
+#define ICEBERG_UNWRAP_OR_FAIL(lhs, rexpr)                                             \
+  ICEBERG_UNWRAP_OR_FAIL_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
+                              rexpr)
+
 }  // namespace iceberg
diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build
@@ -49,6 +49,7 @@ iceberg_tests = {
             'schema_json_test.cc',
             'table_test.cc',
             'test_common.cc',
+            'table_metadata_builder_test.cc',
         ),
     },
     'expression_test': {
@@ -80,9 +81,6 @@ iceberg_tests = {
         ),
     },
     'roaring_test': {'sources': files('roaring_test.cc')},
-    'table_metadata_builder_test': {
-        'sources': files('table_metadata_builder_test.cc'),
-    },
 }
 
 if get_option('rest').enabled()
diff --git a/src/iceberg/test/table_metadata_builder_test.cc b/src/iceberg/test/table_metadata_builder_test.cc
diff --git a/src/iceberg/util/string_util.h b/src/iceberg/util/string_util.h
