fix: review comments

Author: zhjwpku

src/iceberg/json_serde.cc

@@ -1270,22 +1270,15 @@ Result<std::unique_ptr<NameMapping>> NameMappingFromJson(const nlohmann::json& j
   return NameMapping::Make(std::move(mapped_fields));
 }
 
-std::optional<std::string> UpdateMappingFromJsonString(
-    std::string_view mapping_json, const std::map<int32_t, SchemaField>& updates,
+Result<std::string> UpdateMappingFromJsonString(
+    std::string_view mapping_json,
+    const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates,
     const std::multimap<int32_t, int32_t>& adds) {
-  auto json_result = FromJsonString(std::string(mapping_json));
-  if (!json_result) return std::nullopt;
-
-  auto current_mapping = NameMappingFromJson(*json_result);
-  if (!current_mapping) return std::nullopt;
-
-  auto updated_mapping = UpdateMapping(**current_mapping, updates, adds);
-  if (!updated_mapping) return std::nullopt;
-
-  auto json_str = ToJsonString(ToJson(**updated_mapping));
-  if (!json_str) return std::nullopt;
-
-  return std::move(*json_str);
+  ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(std::string(mapping_json)));
+  ICEBERG_ASSIGN_OR_RAISE(auto current_mapping, NameMappingFromJson(json));
+  ICEBERG_ASSIGN_OR_RAISE(auto updated_mapping,
+                          UpdateMapping(*current_mapping, updates, adds));
+  return ToJsonString(ToJson(*updated_mapping));
 }
 
 nlohmann::json ToJson(const TableIdentifier& identifier) {

src/iceberg/json_serde_internal.h

@@ -24,6 +24,7 @@
 #include <optional>
 #include <string>
 #include <string_view>
+#include <unordered_map>
 
 #include <nlohmann/json_fwd.hpp>
 
@@ -354,9 +355,10 @@ ICEBERG_EXPORT Result<std::unique_ptr<NameMapping>> NameMappingFromJson(
 /// \brief Update a name mapping from its JSON string and return updated JSON.
 ///
 /// Parses the JSON, calls UpdateMapping, and serializes the result.
-/// Returns nullopt if any step fails.
-ICEBERG_EXPORT std::optional<std::string> UpdateMappingFromJsonString(
-    std::string_view mapping_json, const std::map<int32_t, SchemaField>& updates,
+/// Returns an error if parsing, mapping update, or serialization fails.
+ICEBERG_EXPORT Result<std::string> UpdateMappingFromJsonString(
+    std::string_view mapping_json,
+    const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates,
     const std::multimap<int32_t, int32_t>& adds);
 
 /// \brief Serializes a `TableIdentifier` object to JSON.

src/iceberg/name_mapping.cc

@@ -326,8 +326,9 @@ class CreateMappingVisitor {
 // Visitor class for updating name mappings with schema changes
 class UpdateMappingVisitor {
  public:
-  UpdateMappingVisitor(const std::map<int32_t, SchemaField>& updates,
-                       const std::multimap<int32_t, int32_t>& adds)
+  UpdateMappingVisitor(
+      const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates,
+      const std::multimap<int32_t, int32_t>& adds)
       : updates_(updates), adds_(adds) {}
 
   Result<std::unique_ptr<MappedFields>> VisitMapping(const NameMapping& mapping) {
@@ -351,15 +352,15 @@ class UpdateMappingVisitor {
 
     // Build update assignments map for removing reassigned names
     std::unordered_map<std::string, int32_t> update_assignments;
-    std::ranges::for_each(field_results, [&](const auto& field) {
+    for (const auto& field : field_results) {
       if (field.field_id.has_value()) {
         auto update_it = updates_.find(field.field_id.value());
         if (update_it != updates_.end()) {
-          update_assignments.emplace(std::string(update_it->second.name()),
+          update_assignments.emplace(std::string(update_it->second->name()),
                                      field.field_id.value());
         }
       }
-    });
+    }
 
     // Remove reassigned names from all fields
     for (auto& field : field_results) {
@@ -375,7 +376,7 @@ class UpdateMappingVisitor {
     if (field.field_id.has_value()) {
       auto update_it = updates_.find(field.field_id.value());
       if (update_it != updates_.end()) {
-        field_names.insert(std::string(update_it->second.name()));
+        field_names.insert(std::string(update_it->second->name()));
       }
     }
 
@@ -408,7 +409,7 @@ class UpdateMappingVisitor {
     for (auto it = range.first; it != range.second; ++it) {
       auto update_it = updates_.find(it->second);
       if (update_it != updates_.end()) {
-        fields_to_add.push_back(&update_it->second);
+        fields_to_add.push_back(update_it->second.get());
       }
     }
 
@@ -471,7 +472,7 @@ class UpdateMappingVisitor {
     };
   }
 
-  const std::map<int32_t, SchemaField>& updates_;
+  const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates_;
   const std::multimap<int32_t, int32_t>& adds_;
 };
 
@@ -488,7 +489,8 @@ Result<std::unique_ptr<NameMapping>> CreateMapping(const Schema& schema) {
 }
 
 Result<std::unique_ptr<NameMapping>> UpdateMapping(
-    const NameMapping& mapping, const std::map<int32_t, SchemaField>& updates,
+    const NameMapping& mapping,
+    const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates,
     const std::multimap<int32_t, int32_t>& adds) {
   UpdateMappingVisitor visitor(updates, adds);
   auto result = visitor.VisitMapping(mapping);

src/iceberg/name_mapping.h

@@ -151,7 +151,8 @@ ICEBERG_EXPORT Result<std::unique_ptr<NameMapping>> CreateMapping(const Schema&
 /// \return an updated mapping with names added to renamed fields and the mapping extended
 /// for new fields
 ICEBERG_EXPORT Result<std::unique_ptr<NameMapping>> UpdateMapping(
-    const NameMapping& mapping, const std::map<int32_t, SchemaField>& updates,
+    const NameMapping& mapping,
+    const std::unordered_map<int32_t, std::shared_ptr<SchemaField>>& updates,
     const std::multimap<int32_t, int32_t>& adds);
 
 }  // namespace iceberg

src/iceberg/update/update_schema.cc

@@ -601,22 +601,17 @@ Result<UpdateSchema::ApplyResult> UpdateSchema::Apply() {
 
   auto mapping_it = properties.find(std::string(TableProperties::kDefaultNameMapping));
   if (mapping_it != properties.end() && !mapping_it->second.empty()) {
-    std::map<int32_t, SchemaField> updates;
-    for (const auto& [id, field_ptr] : updates_) {
-      updates.emplace(id, *field_ptr);
-    }
     std::multimap<int32_t, int32_t> adds;
     for (const auto& [parent_id, child_ids] : parent_to_added_ids_) {
       std::ranges::for_each(child_ids, [&adds, parent_id](int32_t child_id) {
         adds.emplace(parent_id, child_id);
       });
     }
-    auto updated_mapping_json =
-        UpdateMappingFromJsonString(mapping_it->second, updates, adds);
-    if (updated_mapping_json) {
-      updated_props[std::string(TableProperties::kDefaultNameMapping)] =
-          std::move(*updated_mapping_json);
-    }
+    ICEBERG_ASSIGN_OR_RAISE(
+        auto updated_mapping_json,
+        UpdateMappingFromJsonString(mapping_it->second, updates_, adds));
+    updated_props[std::string(TableProperties::kDefaultNameMapping)] =
+        std::move(updated_mapping_json);
   }
 
   return ApplyResult{.schema = std::move(new_schema),