fix: review comments

Author: zhjwpku

src/iceberg/json_internal.cc

@@ -110,9 +110,9 @@ const std::unordered_set<std::string_view> kValidSnapshotSummaryFields = {
     SnapshotSummaryFields::kTotalEqDeletes,
     SnapshotSummaryFields::kDeletedDuplicatedFiles,
     SnapshotSummaryFields::kChangedPartitionCountProp,
-    SnapshotSummaryFields::kWAPID,
-    SnapshotSummaryFields::kPublishedWAPID,
-    SnapshotSummaryFields::kSourceSnapshotID,
+    SnapshotSummaryFields::kWAPId,
+    SnapshotSummaryFields::kPublishedWAPId,
+    SnapshotSummaryFields::kSourceSnapshotId,
     SnapshotSummaryFields::kEngineName,
     SnapshotSummaryFields::kEngineVersion};
 
@@ -154,6 +154,14 @@ Result<std::optional<T>> GetJsonValueOptional(const nlohmann::json& json,
   }
 }
 
+template <typename T>
+void SetOptionalField(nlohmann::json& json, std::string_view key,
+                      const std::optional<T>& value) {
+  if (value.has_value()) {
+    json[key] = *value;
+  }
+}
+
 }  // namespace
 
 nlohmann::json ToJson(const SortField& sort_field) {
@@ -302,44 +310,25 @@ nlohmann::json SnapshotRefToJson(const SnapshotRef& ref) {
   json[kType] = SnapshotRefTypeToString(ref.type());
   if (ref.type() == SnapshotRefType::kBranch) {
     const auto& branch = std::get<SnapshotRef::Branch>(ref.retention);
-    if (branch.min_snapshots_to_keep.has_value()) {
-      json[kMinSnapshotsToKeep] = *branch.min_snapshots_to_keep;
-    }
-    if (branch.max_snapshot_age_ms.has_value()) {
-      json[kMaxSnapshotAgeMs] = *branch.max_snapshot_age_ms;
-    }
-    if (branch.max_ref_age_ms.has_value()) {
-      json[kMaxRefAgeMs] = *branch.max_ref_age_ms;
-    }
+    SetOptionalField(json, kMinSnapshotsToKeep, branch.min_snapshots_to_keep);
+    SetOptionalField(json, kMaxSnapshotAgeMs, branch.max_snapshot_age_ms);
+    SetOptionalField(json, kMaxRefAgeMs, branch.max_ref_age_ms);
   } else if (ref.type() == SnapshotRefType::kTag) {
     const auto& tag = std::get<SnapshotRef::Tag>(ref.retention);
-    if (tag.max_ref_age_ms.has_value()) {
-      json[kMaxRefAgeMs] = *tag.max_ref_age_ms;
-    }
+    SetOptionalField(json, kMaxRefAgeMs, tag.max_ref_age_ms);
   }
   return json;
 }
 
 nlohmann::json SnapshotToJson(const Snapshot& snapshot) {
   nlohmann::json json;
   json[kSnapshotId] = snapshot.snapshot_id;
-  if (snapshot.parent_snapshot_id.has_value()) {
-    json[kParentSnapshotId] = *snapshot.parent_snapshot_id;
-  }
+  SetOptionalField(json, kParentSnapshotId, snapshot.parent_snapshot_id);
   json[kSequenceNumber] = snapshot.sequence_number;
   json[kTimestampMs] = snapshot.timestamp_ms;
   json[kManifestList] = snapshot.manifest_list;
-
-  nlohmann::json summary_json;
-  for (const auto& [key, value] : snapshot.summary) {
-    summary_json[key] = value;
-  }
-  json[kSummary] = summary_json;
-
-  if (snapshot.schema_id.has_value()) {
-    json[kSchemaId] = *snapshot.schema_id;
-  }
-
+  json[kSummary] = snapshot.summary;
+  SetOptionalField(json, kSchemaId, snapshot.schema_id);
   return json;
 }
 

src/iceberg/snapshot.h

@@ -48,8 +48,6 @@ ICEBERG_EXPORT constexpr std::string_view SnapshotRefTypeToString(
       return "branch";
     case SnapshotRefType::kTag:
       return "tag";
-    default:
-      return "invalid";
   }
 }
 /// \brief Get the relative snapshot reference type from name
@@ -198,11 +196,11 @@ struct SnapshotSummaryFields {
   /// Other Fields, see https://iceberg.apache.org/spec/#other-fields
 
   /// \brief The Write-Audit-Publish id of a staged snapshot
-  inline static const std::string kWAPID = "wap.id";
+  inline static const std::string kWAPId = "wap.id";
   /// \brief The Write-Audit-Publish id of a snapshot already been published
-  inline static const std::string kPublishedWAPID = "published-wap-id";
+  inline static const std::string kPublishedWAPId = "published-wap-id";
   /// \brief The original id of a cherry-picked snapshot
-  inline static const std::string kSourceSnapshotID = "source-snapshot-id";
+  inline static const std::string kSourceSnapshotId = "source-snapshot-id";
   /// \brief Name of the engine that created the snapshot
   inline static const std::string kEngineName = "engine-name";
   /// \brief Version of the engine that created the snapshot

test/json_internal_test.cc

@@ -32,6 +32,7 @@
 #include "iceberg/sort_order.h"
 #include "iceberg/transform.h"
 #include "iceberg/util/formatter.h"  // IWYU pragma: keep
+#include "matchers.h"
 
 namespace iceberg {
 
@@ -235,9 +236,8 @@ TEST(JsonInternalTest, SnapshotFromJsonWithInvalidSummary) {
   auto result = SnapshotFromJson(invalid_json_snapshot);
   ASSERT_FALSE(result.has_value());
 
-  EXPECT_EQ(result.error().kind, ErrorKind::kJsonParseError);
-  EXPECT_TRUE(result.error().message.find("Invalid snapshot summary field") !=
-              std::string::npos);
+  EXPECT_THAT(result, IsError(ErrorKind::kJsonParseError));
+  EXPECT_THAT(result, HasErrorMessage("Invalid snapshot summary field"));
 }
 
 }  // namespace iceberg