Revert "fix: address PR feedback for table requirement validations"

This reverts commit 6ba8daf.

Author: shangxinli

src/iceberg/table_requirement.cc

@@ -55,32 +55,32 @@ Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
   // Validate that a reference (branch or tag) points to the expected snapshot ID
 
   if (base == nullptr) {
-    // For new tables (null base), only succeed if we're not expecting a specific snapshot
-    if (snapshot_id_.has_value()) {
-      return CommitFailed(
-          "Requirement failed: branch or tag '{}' is missing, expected {}", ref_name_,
-          snapshot_id_.value());
-    }
-    return {};
+    return CommitFailed("Requirement failed: current table metadata is missing");
   }
 
   auto it = base->refs.find(ref_name_);
-  if (it != base->refs.end()) {
-    // Reference exists
-    if (!snapshot_id_.has_value()) {
-      // A null snapshot ID means the ref should not exist already
-      return CommitFailed(
-          "Requirement failed: reference '{}' was created concurrently", ref_name_);
-    } else if (snapshot_id_.value() != it->second->snapshot_id) {
+
+  // If snapshot_id is nullopt, the reference should not exist
+  if (!snapshot_id_.has_value()) {
+    if (it != base->refs.end()) {
       return CommitFailed(
-          "Requirement failed: reference '{}' has changed: expected id {} != {}",
-          ref_name_, snapshot_id_.value(), it->second->snapshot_id);
+          "Requirement failed: reference '{}' should not exist but found snapshot ID {}",
+          ref_name_, it->second->snapshot_id);
     }
-  } else if (snapshot_id_.has_value()) {
-    // Reference does not exist but snapshot_id is specified
+    return {};
+  }
+
+  // snapshot_id has a value, so the reference should exist and match
+  if (it == base->refs.end()) {
+    return CommitFailed("Requirement failed: reference '{}' is missing in table metadata",
+                        ref_name_);
+  }
+
+  if (it->second->snapshot_id != snapshot_id_.value()) {
     return CommitFailed(
-        "Requirement failed: branch or tag '{}' is missing, expected {}", ref_name_,
-        snapshot_id_.value());
+        "Requirement failed: reference '{}' snapshot ID does not match (expected={}, "
+        "actual={})",
+        ref_name_, snapshot_id_.value(), it->second->snapshot_id);
   }
 
   return {};
@@ -89,7 +89,11 @@ 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
 
-  if (base && base->last_column_id != last_assigned_field_id_) {
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
+
+  if (base->last_column_id != last_assigned_field_id_) {
     return CommitFailed(
         "Requirement failed: last assigned field ID does not match (expected={}, "
         "actual={})",
@@ -123,7 +127,11 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
 Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
   // Validate that the last assigned partition ID matches the expected value
 
-  if (base && base->last_partition_id != last_assigned_partition_id_) {
+  if (base == nullptr) {
+    return CommitFailed("Requirement failed: current table metadata is missing");
+  }
+
+  if (base->last_partition_id != last_assigned_partition_id_) {
     return CommitFailed(
         "Requirement failed: last assigned partition ID does not match (expected={}, "
         "actual={})",

src/iceberg/test/table_metadata_builder_test.cc

@@ -299,23 +299,23 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDMismatch) {
 
   auto status = requirement.Validate(base_metadata_.get());
   EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
-  EXPECT_THAT(status, HasErrorMessage("has changed"));
+  EXPECT_THAT(status, HasErrorMessage("snapshot ID does not match"));
 }
 
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDRefMissing) {
   table::AssertRefSnapshotID requirement("missing-ref", 100);
 
   auto status = requirement.Validate(base_metadata_.get());
   EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
-  EXPECT_THAT(status, HasErrorMessage("is missing"));
+  EXPECT_THAT(status, HasErrorMessage("missing in table metadata"));
 }
 
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNullBase) {
   table::AssertRefSnapshotID requirement("main", 100);
 
   auto status = requirement.Validate(nullptr);
   EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
-  EXPECT_THAT(status, HasErrorMessage("is missing"));
+  EXPECT_THAT(status, HasErrorMessage("metadata is missing"));
 }
 
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptSuccess) {
@@ -336,7 +336,7 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertRefSnapshotIDNulloptButEx
 
   auto status = requirement.Validate(base_metadata_.get());
   EXPECT_THAT(status, IsError(ErrorKind::kCommitFailed));
-  EXPECT_THAT(status, HasErrorMessage("created concurrently"));
+  EXPECT_THAT(status, HasErrorMessage("should not exist"));
 }
 
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdSuccess) {
@@ -358,8 +358,9 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdMismat
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedFieldIdNullBase) {
   table::AssertLastAssignedFieldId requirement(10);
 
-  // Null base is allowed (for new tables)
-  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) {
@@ -381,8 +382,9 @@ TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdMi
 TEST_F(TableMetadataBuilderTest, TableRequirementAssertLastAssignedPartitionIdNullBase) {
   table::AssertLastAssignedPartitionId requirement(5);
 
-  // Null base is allowed (for new tables)
-  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) {