resolve review comments

Signed-off-by: Junwang Zhao <[email protected]>

Author: zhjwpku

src/iceberg/json_internal.cc

@@ -171,16 +171,17 @@ void SetOptionalField(nlohmann::json& json, std::string_view key,
   }
 }
 
-std::string DumpJsonNoExcept(const nlohmann::json& json) {
-  return json.dump(-1, ' ', false, nlohmann::detail::error_handler_t::ignore);
+std::string SafeDumpJson(const nlohmann::json& json) {
+  return json.dump(/*indent=*/-1, /*indent_char=*/' ', /*ensure_ascii=*/false,
+                   nlohmann::detail::error_handler_t::ignore);
 }
 
 template <typename T>
 Result<T> GetJsonValueImpl(const nlohmann::json& json, std::string_view key) {
   try {
     return json.at(key).get<T>();
   } catch (const std::exception& ex) {
-    return JsonParseError("Failed to parse '{}' from {}: {}", key, DumpJsonNoExcept(json),
+    return JsonParseError("Failed to parse '{}' from {}: {}", key, SafeDumpJson(json),
                           ex.what());
   }
 }
@@ -197,7 +198,7 @@ Result<std::optional<T>> GetJsonValueOptional(const nlohmann::json& json,
 template <typename T>
 Result<T> GetJsonValue(const nlohmann::json& json, std::string_view key) {
   if (!json.contains(key)) {
-    return JsonParseError("Missing '{}' in {}", key, DumpJsonNoExcept(json));
+    return JsonParseError("Missing '{}' in {}", key, SafeDumpJson(json));
   }
   return GetJsonValueImpl<T>(json, key);
 }
@@ -248,7 +249,7 @@ Result<std::vector<T>> FromJsonList(
     ICEBERG_ASSIGN_OR_RAISE(auto list_json, GetJsonValue<nlohmann::json>(json, key));
     if (!list_json.is_array()) {
       return JsonParseError("Cannot parse '{}' from non-array: {}", key,
-                            DumpJsonNoExcept(list_json));
+                            SafeDumpJson(list_json));
     }
     for (const auto& entry_json : list_json) {
       ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(entry_json));
@@ -273,7 +274,7 @@ Result<std::vector<std::shared_ptr<T>>> FromJsonList(
     ICEBERG_ASSIGN_OR_RAISE(auto list_json, GetJsonValue<nlohmann::json>(json, key));
     if (!list_json.is_array()) {
       return JsonParseError("Cannot parse '{}' from non-array: {}", key,
-                            DumpJsonNoExcept(list_json));
+                            SafeDumpJson(list_json));
     }
     for (const auto& entry_json : list_json) {
       ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(entry_json));
@@ -322,16 +323,16 @@ Result<std::unordered_map<std::string, T>> FromJsonMap(
       try {
         return json.get<std::string>();
       } catch (const std::exception& ex) {
-        return JsonParseError("Cannot parse {} to a string value: {}",
-                              DumpJsonNoExcept(json), ex.what());
+        return JsonParseError("Cannot parse {} to a string value: {}", SafeDumpJson(json),
+                              ex.what());
       }
     }) {
   std::unordered_map<std::string, T> map{};
   if (json.contains(key)) {
     ICEBERG_ASSIGN_OR_RAISE(auto map_json, GetJsonValue<nlohmann::json>(json, key));
     if (!map_json.is_object()) {
       return JsonParseError("Cannot parse '{}' from non-object: {}", key,
-                            DumpJsonNoExcept(map_json));
+                            SafeDumpJson(map_json));
     }
     for (const auto& [key, value] : map_json.items()) {
       ICEBERG_ASSIGN_OR_RAISE(auto entry, from_json(value));
@@ -636,8 +637,7 @@ Result<std::unique_ptr<Schema>> SchemaFromJson(const nlohmann::json& json) {
   ICEBERG_ASSIGN_OR_RAISE(auto type, TypeFromJson(json));
 
   if (type->type_id() != TypeId::kStruct) [[unlikely]] {
-    return JsonParseError("Schema must be a struct type, but got {}",
-                          DumpJsonNoExcept(json));
+    return JsonParseError("Schema must be a struct type, but got {}", SafeDumpJson(json));
   }
 
   auto& struct_type = static_cast<StructType&>(*type);
@@ -746,11 +746,11 @@ Result<std::unique_ptr<Snapshot>> SnapshotFromJson(const nlohmann::json& json) {
       }
       if (!value.is_string()) {
         return JsonParseError("Invalid snapshot summary field value: {}",
-                              DumpJsonNoExcept(value));
+                              SafeDumpJson(value));
       }
       if (key == SnapshotSummaryFields::kOperation &&
           !kValidDataOperation.contains(value.get<std::string>())) {
-        return JsonParseError("Invalid snapshot operation: {}", DumpJsonNoExcept(value));
+        return JsonParseError("Invalid snapshot operation: {}", SafeDumpJson(value));
       }
       summary[key] = value.get<std::string>();
     }
@@ -980,7 +980,7 @@ Result<std::shared_ptr<Schema>> ParseSchemas(
                             GetJsonValue<nlohmann::json>(json, kSchemas));
     if (!schema_array.is_array()) {
       return JsonParseError("Cannot parse schemas from non-array: {}",
-                            DumpJsonNoExcept(schema_array));
+                            SafeDumpJson(schema_array));
     }
 
     ICEBERG_ASSIGN_OR_RAISE(current_schema_id,
@@ -995,7 +995,7 @@ Result<std::shared_ptr<Schema>> ParseSchemas(
     }
     if (!current_schema) {
       return JsonParseError("Cannot find schema with {}={} from {}", kCurrentSchemaId,
-                            current_schema_id.value(), DumpJsonNoExcept(schema_array));
+                            current_schema_id.value(), SafeDumpJson(schema_array));
     }
   } else {
     if (format_version != 1) {
@@ -1026,7 +1026,7 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
                             GetJsonValue<nlohmann::json>(json, kPartitionSpecs));
     if (!spec_array.is_array()) {
       return JsonParseError("Cannot parse partition specs from non-array: {}",
-                            DumpJsonNoExcept(spec_array));
+                            SafeDumpJson(spec_array));
     }
     ICEBERG_ASSIGN_OR_RAISE(default_spec_id, GetJsonValue<int32_t>(json, kDefaultSpecId));
 
@@ -1045,7 +1045,7 @@ Status ParsePartitionSpecs(const nlohmann::json& json, int8_t format_version,
                             GetJsonValue<nlohmann::json>(json, kPartitionSpec));
     if (!partition_spec_json.is_array()) {
       return JsonParseError("Cannot parse v1 partition spec from non-array: {}",
-                            DumpJsonNoExcept(partition_spec_json));
+                            SafeDumpJson(partition_spec_json));
     }
 
     int32_t next_partition_field_id = PartitionSpec::kLegacyPartitionDataIdStart;
@@ -1104,7 +1104,7 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version,
 Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::json& json) {
   if (!json.is_object()) {
     return JsonParseError("Cannot parse metadata from a non-object: {}",
-                          DumpJsonNoExcept(json));
+                          SafeDumpJson(json));
   }
 
   auto table_metadata = std::make_unique<TableMetadata>();
@@ -1216,8 +1216,9 @@ Result<std::unique_ptr<TableMetadata>> TableMetadataFromJson(const nlohmann::jso
 }
 
 Result<nlohmann::json> FromJsonString(const std::string& json_string) {
-  auto json = nlohmann::json::parse(json_string, nullptr, false);
-  if (json.is_discarded()) {
+  auto json =
+      nlohmann::json::parse(json_string, /*cb=*/nullptr, /*allow_exceptions=*/false);
+  if (json.is_discarded()) [[unlikely]] {
     return JsonParseError("Failed to parse JSON string: {}", json_string);
   }
   return json;