feat: implement UpdateMapping and apply meta change to UpdateSchema

[zhjwpku]

No description provided.

[evindj]

This probably works but it seems the pattern in this file is to use Result() for *FromJson. It is also unclear what the intend of the method is, it seems from reading the code that we are trying to update the initial json representation with updates.

[zhjwpku]

Changed to the Result pattern.

[wgtmac]

Review Report: PR #561

📄 File: src/iceberg/name_mapping.cc

Java Counterpart: core/src/main/java/org/apache/iceberg/mapping/MappingUtil.java

• Parity Check: ✅ The UpdateMappingVisitor logically matches the Java UpdateMapping visitor. The logic handling additions and reassigned names perfectly mirrors the Java implementation (e.g. !field.field_id.has_value() || assign_it->second != field.field_id.value() corresponds correctly to Java's nullable integer checks assignedId != null && !Objects.equals(assignedId, field.id())).
• Style Check: ⚠️ In UpdateMappingVisitor::VisitFields, std::ranges::for_each is used with a lambda that mutates update_assignments. Standard C++ algorithms are less idiomatic when they perform side-effects on captured external variables. A simple range-based for loop for (const auto& field : field_results) is cleaner and more readable here.
• Logic Check: ✅ Handling of recursive nested structures with std::unique_ptr and implicit conversion to std::shared_ptr is correct and safe.
• Design & Conciseness: ✅ UpdateMappingVisitor does a great job encapsulating the logic internally. std::erase_if correctly translates Sets.difference from Java without reinventing the wheel.

📄 File: src/iceberg/json_serde.cc

Java Counterpart: api/src/main/java/org/apache/iceberg/mapping/NameMappingParser.java

• Parity Check: ✅
• Style Check: ⚠️ UpdateMappingFromJsonString returns std::optional<std::string>, which silently ignores JSON parsing or mapping errors. Returning Result<std::string> would allow callers to log or trace errors if they occur, keeping consistent with the codebase's Result<T> error handling pattern.
• Logic Check: ✅
• Design & Conciseness: ✅ Consolidating mapping deserialization/serialization into UpdateMappingFromJsonString is a concise design that avoids leaking JSON boilerplate into UpdateSchema.

📄 File: src/iceberg/update/update_schema.cc

Java Counterpart: core/src/main/java/org/apache/iceberg/SchemaUpdate.java

• Parity Check: ✅ In Java, updating the name mapping property is done inside SchemaUpdate#commit() via applyChangesToMetadata(). In C++, embedding this property update inside UpdateSchema::Apply() and returning it within updated_props is the correct equivalent pattern.
• Style Check: ✅
• Logic Check: ✅
• Design & Conciseness: ✅

Summary & Recommendation

• Comment. The implementation is functionally sound, adheres tightly to Java parity, and cleanly introduces schema name mapping updates. Consider switching the std::ranges::for_each to a for loop in name_mapping.cc and returning a Result<std::string> instead of std::optional in json_serde.cc for better error traceability, though neither are critical blockers. Excellent work!

[zhjwpku]

Review Report: PR #561

📄 File: src/iceberg/name_mapping.cc

Java Counterpart: core/src/main/java/org/apache/iceberg/mapping/MappingUtil.java

• Parity Check: ✅ The UpdateMappingVisitor logically matches the Java UpdateMapping visitor. The logic handling additions and reassigned names perfectly mirrors the Java implementation (e.g. !field.field_id.has_value() || assign_it->second != field.field_id.value() corresponds correctly to Java's nullable integer checks assignedId != null && !Objects.equals(assignedId, field.id())).
• Style Check: ⚠️ In UpdateMappingVisitor::VisitFields, std::ranges::for_each is used with a lambda that mutates update_assignments. Standard C++ algorithms are less idiomatic when they perform side-effects on captured external variables. A simple range-based for loop for (const auto& field : field_results) is cleaner and more readable here.

Yeah, changed to simple for loop.

• Logic Check: ✅ Handling of recursive nested structures with std::unique_ptr and implicit conversion to std::shared_ptr is correct and safe.
• Design & Conciseness: ✅ UpdateMappingVisitor does a great job encapsulating the logic internally. std::erase_if correctly translates Sets.difference from Java without reinventing the wheel.

📄 File: src/iceberg/json_serde.cc

Java Counterpart: api/src/main/java/org/apache/iceberg/mapping/NameMappingParser.java

• Parity Check: ✅
• Style Check: ⚠️ UpdateMappingFromJsonString returns std::optional<std::string>, which silently ignores JSON parsing or mapping errors. Returning Result<std::string> would allow callers to log or trace errors if they occur, keeping consistent with the codebase's Result<T> error handling pattern.

Changed to Resultstd::string

• Logic Check: ✅
• Design & Conciseness: ✅ Consolidating mapping deserialization/serialization into UpdateMappingFromJsonString is a concise design that avoids leaking JSON boilerplate into UpdateSchema.

📄 File: src/iceberg/update/update_schema.cc

Java Counterpart: core/src/main/java/org/apache/iceberg/SchemaUpdate.java

• Parity Check: ✅ In Java, updating the name mapping property is done inside SchemaUpdate#commit() via applyChangesToMetadata(). In C++, embedding this property update inside UpdateSchema::Apply() and returning it within updated_props is the correct equivalent pattern.
• Style Check: ✅
• Logic Check: ✅
• Design & Conciseness: ✅

Summary & Recommendation

• Comment. The implementation is functionally sound, adheres tightly to Java parity, and cleanly introduces schema name mapping updates. Consider switching the std::ranges::for_each to a for loop in name_mapping.cc and returning a Result<std::string> instead of std::optional in json_serde.cc for better error traceability, though neither are critical blockers. Excellent work!