fix ci

Author: HeartLinked

src/iceberg/table_metadata.cc

@@ -23,8 +23,8 @@
 #include <chrono>
 #include <cstdint>
 #include <format>
-#include <ranges>
 #include <optional>
+#include <ranges>
 #include <string>
 #include <unordered_map>
 
@@ -45,6 +45,21 @@
 
 namespace iceberg {
 
+#define BUILDER_RETURN_IF_ERROR(result)                         \
+  if (auto&& result_name = result; !result_name) [[unlikely]] { \
+    impl_->errors.emplace_back(std::move(result_name.error())); \
+    return *this;                                               \
+  }
+
+#define BUILDER_ASSIGN_OR_RETURN_IMPL(result_name, lhs, rexpr) \
+  auto&& result_name = (rexpr);                                \
+  BUILDER_RETURN_IF_ERROR(result_name)                         \
+  lhs = std::move(result_name.value());
+
+#define BUILDER_ASSIGN_OR_RETURN(lhs, rexpr)                                             \
+  BUILDER_ASSIGN_OR_RETURN_IMPL(ICEBERG_ASSIGN_OR_RAISE_NAME(result_, __COUNTER__), lhs, \
+                                rexpr)
+
 namespace {
 const TimePointMs kInvalidLastUpdatedMs = TimePointMs::min();
 }
@@ -322,7 +337,7 @@ struct TableMetadataBuilder::Impl {
     }
   }
 
-  int32_t reuseOrCreateNewSortOrderId(const SortOrder& new_order) {
+  int32_t ReuseOrCreateNewSortOrderId(const SortOrder& new_order) {
     if (new_order.is_unsorted()) {
       return SortOrder::kUnsortedOrderId;
     }
@@ -457,7 +472,7 @@ TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t order_id
 
 TableMetadataBuilder& TableMetadataBuilder::AddSortOrder(
     std::shared_ptr<SortOrder> order) {
-  int32_t new_order_id = impl_->reuseOrCreateNewSortOrderId(*order);
+  int32_t new_order_id = impl_->ReuseOrCreateNewSortOrderId(*order);
 
   if (impl_->sort_orders_by_id.find(new_order_id) != impl_->sort_orders_by_id.end()) {
     // update last_added_order_id if the order was added in this set of changes (since it
@@ -475,42 +490,20 @@ TableMetadataBuilder& TableMetadataBuilder::AddSortOrder(
     return *this;
   }
 
-  // Get current schema for validation
-  auto schema_result = impl_->metadata.Schema();
-  if (!schema_result) {
-    impl_->errors.emplace_back(
-        ErrorKind::kInvalidArgument,
-        std::format("Cannot find current schema: {}", schema_result.error().message));
-    return *this;
-  }
-
-  auto schema = schema_result.value();
-
-  // Validate the sort order against the schema
-  auto validate_status = order->Validate(*schema);
-  if (!validate_status) {
-    impl_->errors.emplace_back(
-        ErrorKind::kInvalidArgument,
-        std::format("Sort order validation failed: {}", validate_status.error().message));
-    return *this;
-  }
+  // Get current schema and validate the sort order against it
+  BUILDER_ASSIGN_OR_RETURN(auto schema, impl_->metadata.Schema());
+  BUILDER_RETURN_IF_ERROR(order->Validate(*schema));
 
   std::shared_ptr<SortOrder> new_order;
   if (order->is_unsorted()) {
     new_order = SortOrder::Unsorted();
   } else {
-    // TODO(Li Feiyang): rebuild the sort order using new column ids (freshSortOrder)
-    // For now, create a new sort order with the provided fields
-    auto sort_order_result = SortOrder::Make(
-        *schema, new_order_id,
-        std::vector<SortField>(order->fields().begin(), order->fields().end()));
-    if (!sort_order_result) {
-      impl_->errors.emplace_back(ErrorKind::kInvalidArgument,
-                                 std::format("Failed to create sort order: {}",
-                                             sort_order_result.error().message));
-      return *this;
-    }
-    new_order = std::move(sort_order_result.value());
+    // Unlike freshSortOrder from Java impl, we don't use field name from old bound
+    // schema to rebuild the sort order.
+    BUILDER_ASSIGN_OR_RETURN(
+        new_order,
+        SortOrder::Make(new_order_id, std::vector<SortField>(order->fields().begin(),
+                                                             order->fields().end())));
   }
 
   impl_->metadata.sort_orders.push_back(new_order);

src/iceberg/test/table_metadata_builder_test.cc

@@ -203,8 +203,7 @@ TEST(TableMetadataBuilderTest, AddSortOrderInvalid) {
                          SortOrder::Make(1, std::vector<SortField>{invalid_field}));
   builder->AddSortOrder(std::move(order1));
   ASSERT_THAT(builder->Build(), IsError(ErrorKind::kCommitFailed));
-  ASSERT_THAT(builder->Build(),
-              HasErrorMessage("Cannot find source column for sort field"));
+  ASSERT_THAT(builder->Build(), HasErrorMessage("Cannot find source column"));
 
   // 2. Invalid transform (Day transform on string type)
   builder = TableMetadataBuilder::BuildFrom(base.get());
@@ -224,35 +223,38 @@ TEST(TableMetadataBuilderTest, AddSortOrderInvalid) {
                          SortOrder::Make(*schema, 1, std::vector<SortField>{field}));
   builder->AddSortOrder(std::move(order3));
   ASSERT_THAT(builder->Build(), IsError(ErrorKind::kCommitFailed));
-  ASSERT_THAT(builder->Build(), HasErrorMessage("Cannot find current schema"));
+  ASSERT_THAT(builder->Build(), HasErrorMessage("Schema with ID"));
 }
 
-// Test applying TableUpdate to builder
-TEST(TableMetadataBuilderTest, ApplyUpdate) {
-  // Use base metadata with schema for all update tests
+// Test applying AssignUUID update to builder
+TEST(TableMetadataBuilderTest, ApplyAssignUUIDUpdate) {
   auto base = CreateBaseMetadata();
   auto builder = TableMetadataBuilder::BuildFrom(base.get());
 
   // Apply AssignUUID update
   table::AssignUUID uuid_update("apply-uuid");
   uuid_update.ApplyTo(*builder);
 
+  ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build());
+  EXPECT_EQ(metadata->table_uuid, "apply-uuid");
+}
+
+// Test applying AddSortOrder update to builder
+TEST(TableMetadataBuilderTest, ApplyAddSortOrderUpdate) {
+  auto base = CreateBaseMetadata();
+  auto builder = TableMetadataBuilder::BuildFrom(base.get());
+
   // Apply AddSortOrder update
   auto schema = CreateTestSchema();
   SortField sort_field(1, Transform::Identity(), SortDirection::kAscending,
                        NullOrder::kFirst);
   ICEBERG_UNWRAP_OR_FAIL(auto sort_order_unique,
                          SortOrder::Make(*schema, 1, std::vector<SortField>{sort_field}));
   auto sort_order = std::shared_ptr<SortOrder>(std::move(sort_order_unique));
-
   table::AddSortOrder sort_order_update(sort_order);
   sort_order_update.ApplyTo(*builder);
 
-  // TODO(Li Feiyang): Apply more build methods once they are implemented
-
-  // Verify all updates were applied
   ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build());
-  EXPECT_EQ(metadata->table_uuid, "apply-uuid");
   ASSERT_EQ(metadata->sort_orders.size(), 2);
   EXPECT_EQ(metadata->sort_orders[1]->order_id(), 1);
 }

src/iceberg/test/table_update_test.cc

@@ -105,9 +105,8 @@ TEST(TableUpdateTest, AddSortOrderGenerateRequirements) {
   auto schema = CreateTestSchema();
   SortField sort_field(1, Transform::Identity(), SortDirection::kAscending,
                        NullOrder::kFirst);
-  ICEBERG_UNWRAP_OR_FAIL(auto sort_order_unique,
+  ICEBERG_UNWRAP_OR_FAIL(std::shared_ptr<SortOrder> sort_order,
                          SortOrder::Make(*schema, 1, std::vector<SortField>{sort_field}));
-  auto sort_order = std::shared_ptr<SortOrder>(std::move(sort_order_unique));
   table::AddSortOrder update(sort_order);
 
   // New table - no requirements