remove AI-generated comments and fix some missing check

Author: wgtmac

src/iceberg/table_requirement.cc

@@ -21,13 +21,12 @@
 
 #include "iceberg/snapshot.h"
 #include "iceberg/table_metadata.h"
+#include "iceberg/util/macros.h"
 #include "iceberg/util/string_util.h"
 
 namespace iceberg::table {
 
 Status AssertDoesNotExist::Validate(const TableMetadata* base) const {
-  // Validate that the table does not exist
-
   if (base != nullptr) {
     return CommitFailed("Requirement failed: table already exists");
   }
@@ -36,39 +35,39 @@ Status AssertDoesNotExist::Validate(const TableMetadata* base) const {
 }
 
 Status AssertUUID::Validate(const TableMetadata* base) const {
-  // Validate that the table UUID matches the expected value
-
   if (base == nullptr) {
     return CommitFailed("Requirement failed: current table metadata is missing");
   }
 
   if (!StringUtils::EqualsIgnoreCase(base->table_uuid, uuid_)) {
     return CommitFailed(
-        "Requirement failed: table UUID does not match (expected='{}', actual='{}')",
-        uuid_, base->table_uuid);
+        "Requirement failed: table UUID does not match, expected {} != {}", uuid_,
+        base->table_uuid);
   }
 
   return {};
 }
 
 Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
-  // Validate that a reference (branch or tag) points to the expected snapshot ID
-  // Matches Java implementation logic
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
 
-  auto it = base->refs.find(ref_name_);
-  if (it != base->refs.end()) {
-    // Reference exists
+  if (auto ref = base->refs.find(ref_name_); ref != base->refs.cend()) {
+    const auto& snapshot_ref = ref->second;
+    ICEBERG_DCHECK(snapshot_ref != nullptr, "Snapshot reference is null");
+    std::string_view type =
+        snapshot_ref->type() == SnapshotRefType::kBranch ? "branch" : "tag";
     if (!snapshot_id_.has_value()) {
       // A null snapshot ID means the ref should not exist already
-      return CommitFailed("Requirement failed: reference '{}' was created concurrently",
+      return CommitFailed("Requirement failed: {} '{}' was created concurrently", type,
                           ref_name_);
-    } else if (snapshot_id_.value() != it->second->snapshot_id) {
-      return CommitFailed(
-          "Requirement failed: reference '{}' has changed: expected id {} != {}",
-          ref_name_, snapshot_id_.value(), it->second->snapshot_id);
+    } else if (snapshot_id_.value() != snapshot_ref->snapshot_id) {
+      return CommitFailed("Requirement failed: {} '{}' has changed: expected id {} != {}",
+                          type, ref_name_, snapshot_id_.value(),
+                          snapshot_ref->snapshot_id);
     }
   } else if (snapshot_id_.has_value()) {
-    // Reference does not exist but snapshot_id is specified
     return CommitFailed("Requirement failed: branch or tag '{}' is missing, expected {}",
                         ref_name_, snapshot_id_.value());
   }
@@ -77,22 +76,20 @@ Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
 }
 
 Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
-  // Validate that the last assigned field ID matches the expected value
-  // Allows base to be null for new tables
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
 
   if (base && base->last_column_id != last_assigned_field_id_) {
     return CommitFailed(
-        "Requirement failed: last assigned field ID does not match (expected={}, "
-        "actual={})",
+        "Requirement failed: last assigned field ID does not match, expected {} != {}",
         last_assigned_field_id_, base->last_column_id);
   }
 
   return {};
 }
 
 Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
-  // Validate that the current schema ID matches the one used when the metadata was read
-
   if (base == nullptr) {
     return CommitFailed("Requirement failed: current table metadata is missing");
   }
@@ -104,43 +101,46 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
 
   if (base->current_schema_id.value() != schema_id_) {
     return CommitFailed(
-        "Requirement failed: current schema ID does not match (expected={}, actual={})",
+        "Requirement failed: current schema ID does not match, expected {} != {}",
         schema_id_, base->current_schema_id.value());
   }
 
   return {};
 }
 
 Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
-  // Validate that the last assigned partition ID matches the expected value
-  // Allows base to be null for new tables
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
 
   if (base && base->last_partition_id != last_assigned_partition_id_) {
     return CommitFailed(
-        "Requirement failed: last assigned partition ID does not match (expected={}, "
-        "actual={})",
+        "Requirement failed: last assigned partition ID does not match, expected {} != "
+        "{}",
         last_assigned_partition_id_, base->last_partition_id);
   }
 
   return {};
 }
 
 Status AssertDefaultSpecID::Validate(const TableMetadata* base) const {
-  // Validate that the default partition spec ID matches the expected value
-  // Matches Java implementation - assumes base is never null
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
 
   if (base->default_spec_id != spec_id_) {
     return CommitFailed(
-        "Requirement failed: default partition spec changed: expected id {} != {}",
+        "Requirement failed: default partition spec changed, expected id {} != {}",
         spec_id_, base->default_spec_id);
   }
 
   return {};
 }
 
 Status AssertDefaultSortOrderID::Validate(const TableMetadata* base) const {
-  // Validate that the default sort order ID matches the expected value
-  // Matches Java implementation - assumes base is never null
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
 
   if (base->default_sort_order_id != sort_order_id_) {
     return CommitFailed(

src/iceberg/test/table_metadata_builder_test.cc

@@ -328,7 +328,6 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButEx
   ref->retention = SnapshotRef::Branch{};
   base_metadata_->refs["main"] = ref;
 
-  // Ref should not exist, but it does
   table::AssertRefSnapshotID requirement("main", std::nullopt);
 
   auto status = requirement.Validate(base_metadata_.get());
@@ -355,8 +354,9 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) {
   table::AssertLastAssignedFieldId requirement(10);
 
-  // Null base is now allowed (for new tables) - should succeed
-  ASSERT_THAT(requirement.Validate(nullptr), IsOk());
+  auto status = requirement.Validate(nullptr);
+  EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
+  EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
 }
 
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdSuccess) {
@@ -378,8 +378,9 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMi
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) {
   table::AssertLastAssignedPartitionId requirement(5);
 
-  // Null base is now allowed (for new tables) - should succeed
-  ASSERT_THAT(requirement.Validate(nullptr), IsOk());
+  auto status = requirement.Validate(nullptr);
+  EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
+  EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
 }
 
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDSuccess) {
@@ -398,11 +399,6 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSpecIDMismatch) {
   EXPECT_THAT(status, HasErrorMessage("spec changed"));
 }
 
-// Removed TableRequirementAssertDefaultSpecIDNullBase test
-// Java implementation doesn't check for null base, so passing nullptr would cause
-// undefined behavior This matches Java's assumption that base is never null when Validate
-// is called
-
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDSuccess) {
   base_metadata_->default_sort_order_id = 2;
   table::AssertDefaultSortOrderID requirement(2);
@@ -419,11 +415,6 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertDefaultSortOrderIDMismatc
   EXPECT_THAT(status, HasErrorMessage("sort order changed"));
 }
 
-// Removed TableRequirementAssertDefaultSortOrderIDNullBase test
-// Java implementation doesn't check for null base, so passing nullptr would cause
-// undefined behavior This matches Java's assumption that base is never null when Validate
-// is called
-
 // ============================================================================
 // Integration Tests - End-to-End Workflow
 // ============================================================================