Skip to content

Commit 8847ca8

Browse files
zhjwpkuzhjwpku
committed
use ToJson for Snapshot serialization
1 parent ef171a4 commit 8847ca8

File tree

3 files changed

+59
-57
lines changed

3 files changed

+59
-57
lines changed

‎src/iceberg/json_internal.cc‎

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ nlohmann::json SchemaToJson(const Schema& schema) {
304304
return json;
305305
}
306306

307-
nlohmann::json SnapshotRefToJson(const SnapshotRef& ref) {
307+
nlohmann::json ToJson(const SnapshotRef& ref) {
308308
nlohmann::json json;
309309
json[kSnapshotId] = ref.snapshot_id;
310310
json[kType] = SnapshotRefTypeToString(ref.type());
@@ -320,14 +320,17 @@ nlohmann::json SnapshotRefToJson(const SnapshotRef& ref) {
320320
return json;
321321
}
322322

323-
nlohmann::json SnapshotToJson(const Snapshot& snapshot) {
323+
nlohmann::json ToJson(const Snapshot& snapshot) {
324324
nlohmann::json json;
325325
json[kSnapshotId] = snapshot.snapshot_id;
326326
SetOptionalField(json, kParentSnapshotId, snapshot.parent_snapshot_id);
327327
json[kSequenceNumber] = snapshot.sequence_number;
328328
json[kTimestampMs] = snapshot.timestamp_ms;
329329
json[kManifestList] = snapshot.manifest_list;
330-
json[kSummary] = snapshot.summary;
330+
// If there is an operation, write the summary map
331+
if (snapshot.operation().has_value()) {
332+
json[kSummary] = snapshot.summary;
333+
}
331334
SetOptionalField(json, kSchemaId, snapshot.schema_id);
332335
return json;
333336
}
@@ -558,30 +561,32 @@ Result<std::unique_ptr<Snapshot>> SnapshotFromJson(const nlohmann::json& json) {
558561
GetJsonValueOptional<int64_t>(json, kParentSnapshotId));
559562

560563
ICEBERG_ASSIGN_OR_RAISE(auto summary_json,
561-
GetJsonValue<nlohmann::json>(json, kSummary));
564+
GetJsonValueOptional<nlohmann::json>(json, kSummary));
562565
std::unordered_map<std::string, std::string> summary;
563-
for (const auto& [key, value] : summary_json.items()) {
564-
if (!kValidSnapshotSummaryFields.contains(key)) {
565-
return unexpected<Error>({
566-
.kind = ErrorKind::kJsonParseError,
567-
.message = std::format("Invalid snapshot summary field: {}", key),
568-
});
569-
}
570-
if (!value.is_string()) {
571-
return unexpected<Error>({
572-
.kind = ErrorKind::kJsonParseError,
573-
.message =
574-
std::format("Invalid snapshot summary field value: {}", value.dump()),
575-
});
576-
}
577-
if (key == SnapshotSummaryFields::kOperation &&
578-
!kValidDataOperation.contains(value.get<std::string>())) {
579-
return unexpected<Error>({
580-
.kind = ErrorKind::kJsonParseError,
581-
.message = std::format("Invalid snapshot operation: {}", value.dump()),
582-
});
566+
if (summary_json.has_value()) {
567+
for (const auto& [key, value] : summary_json->items()) {
568+
if (!kValidSnapshotSummaryFields.contains(key)) {
569+
return unexpected<Error>({
570+
.kind = ErrorKind::kJsonParseError,
571+
.message = std::format("Invalid snapshot summary field: {}", key),
572+
});
573+
}
574+
if (!value.is_string()) {
575+
return unexpected<Error>({
576+
.kind = ErrorKind::kJsonParseError,
577+
.message =
578+
std::format("Invalid snapshot summary field value: {}", value.dump()),
579+
});
580+
}
581+
if (key == SnapshotSummaryFields::kOperation &&
582+
!kValidDataOperation.contains(value.get<std::string>())) {
583+
return unexpected<Error>({
584+
.kind = ErrorKind::kJsonParseError,
585+
.message = std::format("Invalid snapshot operation: {}", value.dump()),
586+
});
587+
}
588+
summary[key] = value.get<std::string>();
583589
}
584-
summary[key] = value.get<std::string>();
585590
}
586591

587592
ICEBERG_ASSIGN_OR_RAISE(auto schema_id, GetJsonValueOptional<int32_t>(json, kSchemaId));

‎src/iceberg/json_internal.h‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,17 +88,17 @@ nlohmann::json TypeToJson(const Type& type);
8888
/// \return The JSON representation of the field.
8989
nlohmann::json FieldToJson(const SchemaField& field);
9090

91-
/// \brief Convert an Iceberg SnapshotRef to JSON.
91+
/// \brief Serializes a `SnapshotRef` object to JSON.
9292
///
93-
/// \param[in] snapshot_ref The Iceberg snapshot reference to convert.
94-
/// \return The JSON representation of the snapshot reference.
95-
nlohmann::json SnapshotRefToJson(const SnapshotRef& snapshot_ref);
93+
/// \param[in] snapshot_ref The `SnapshotRef` object to be serialized.
94+
/// \return A JSON object representing the `SnapshotRef`.
95+
nlohmann::json ToJson(const SnapshotRef& snapshot_ref);
9696

97-
/// \brief Convert an Iceberg Snapshot to JSON.
97+
/// \brief Serializes a `Snapshot` object to JSON.
9898
///
99-
/// \param[in] snapshot The Iceberg snapshot to convert.
100-
/// \return The JSON representation of the snapshot.
101-
nlohmann::json SnapshotToJson(const Snapshot& snapshot);
99+
/// \param[in] snapshot The `Snapshot` object to be serialized.
100+
/// \return A JSON object representing the `snapshot`.
101+
nlohmann::json ToJson(const Snapshot& snapshot);
102102

103103
/// \brief Convert JSON to an Iceberg Schema.
104104
///
@@ -166,16 +166,16 @@ Result<std::unique_ptr<PartitionField>> PartitionFieldFromJson(
166166
Result<std::unique_ptr<PartitionSpec>> PartitionSpecFromJson(
167167
const std::shared_ptr<Schema>& schema, const nlohmann::json& json);
168168

169-
/// \brief Convert JSON to an Iceberg SnapshotRef.
169+
/// \brief Deserializes a JSON object into a `SnapshotRef` object.
170170
///
171-
/// \param[in] json The JSON representation of the snapshot reference.
172-
/// \return The Iceberg snapshot reference or an error if the conversion fails.
171+
/// \param[in] json The JSON object representing a `SnapshotRef`.
172+
/// \return A `SnapshotRef` object or an error if the conversion fails.
173173
Result<std::unique_ptr<SnapshotRef>> SnapshotRefFromJson(const nlohmann::json& json);
174174

175-
/// \brief Convert JSON to an Iceberg Snapshot.
175+
/// \brief Deserializes a JSON object into a `Snapshot` object.
176176
///
177177
/// \param[in] json The JSON representation of the snapshot.
178-
/// \return The Iceberg snapshot or an error if the conversion fails.
178+
/// \return A `Snapshot` object or an error if the conversion fails.
179179
Result<std::unique_ptr<Snapshot>> SnapshotFromJson(const nlohmann::json& json);
180180

181181
} // namespace iceberg

‎test/json_internal_test.cc‎

Lines changed: 16 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include <iceberg/result.h>
2626
#include <nlohmann/json.hpp>
2727

28+
#include "gmock/gmock.h"
2829
#include "iceberg/partition_spec.h"
2930
#include "iceberg/schema.h"
3031
#include "iceberg/snapshot.h"
@@ -56,6 +57,16 @@ Result<std::unique_ptr<PartitionField>> FromJsonHelper(const nlohmann::json& jso
5657
return PartitionFieldFromJson(json);
5758
}
5859

60+
template <>
61+
Result<std::unique_ptr<SnapshotRef>> FromJsonHelper(const nlohmann::json& json) {
62+
return SnapshotRefFromJson(json);
63+
}
64+
65+
template <>
66+
Result<std::unique_ptr<Snapshot>> FromJsonHelper(const nlohmann::json& json) {
67+
return SnapshotFromJson(json);
68+
}
69+
5970
// Helper function to reduce duplication in testing
6071
template <typename T>
6172
void TestJsonConversion(const T& obj, const nlohmann::json& expected_json) {
@@ -117,7 +128,8 @@ TEST(JsonPartitionTest, PartitionFieldFromJsonMissingField) {
117128

118129
auto result = PartitionFieldFromJson(invalid_json);
119130
EXPECT_FALSE(result.has_value());
120-
EXPECT_EQ(result.error().kind, ErrorKind::kJsonParseError);
131+
EXPECT_THAT(result, IsError(ErrorKind::kJsonParseError));
132+
EXPECT_THAT(result, HasErrorMessage("Missing 'source-id'"));
121133
}
122134

123135
TEST(JsonPartitionTest, PartitionSpec) {
@@ -162,12 +174,7 @@ TEST(JsonInternalTest, SnapshotRefBranch) {
162174
"max-snapshot-age-ms":123456789,
163175
"max-ref-age-ms":987654321})"_json;
164176

165-
auto json = SnapshotRefToJson(ref);
166-
EXPECT_EQ(expected_json, json) << "JSON conversion mismatch.";
167-
168-
auto obj_ex = SnapshotRefFromJson(expected_json);
169-
EXPECT_TRUE(obj_ex.has_value()) << "Failed to deserialize JSON.";
170-
EXPECT_EQ(ref, *obj_ex.value()) << "Deserialized object mismatch.";
177+
TestJsonConversion(ref, expected_json);
171178
}
172179

173180
TEST(JsonInternalTest, SnapshotRefTag) {
@@ -179,12 +186,7 @@ TEST(JsonInternalTest, SnapshotRefTag) {
179186
"type":"tag",
180187
"max-ref-age-ms":54321})"_json;
181188

182-
auto json = SnapshotRefToJson(ref);
183-
EXPECT_EQ(expected_json, json) << "JSON conversion mismatch.";
184-
185-
auto obj_ex = SnapshotRefFromJson(expected_json);
186-
EXPECT_TRUE(obj_ex.has_value()) << "Failed to deserialize JSON.";
187-
EXPECT_EQ(ref, *obj_ex.value()) << "Deserialized object mismatch.";
189+
TestJsonConversion(ref, expected_json);
188190
}
189191

190192
TEST(JsonInternalTest, Snapshot) {
@@ -213,12 +215,7 @@ TEST(JsonInternalTest, Snapshot) {
213215
},
214216
"schema-id":42})"_json;
215217

216-
auto json = SnapshotToJson(snapshot);
217-
EXPECT_EQ(expected_json, json) << "JSON conversion mismatch.";
218-
219-
auto obj_ex = SnapshotFromJson(expected_json);
220-
EXPECT_TRUE(obj_ex.has_value()) << "Failed to deserialize JSON.";
221-
EXPECT_EQ(snapshot, *obj_ex.value()) << "Deserialized object mismatch.";
218+
TestJsonConversion(snapshot, expected_json);
222219
}
223220

224221
TEST(JsonInternalTest, SnapshotFromJsonWithInvalidSummary) {

0 commit comments

Comments
 (0)