Core: Snapshot summary map must have operation key (#11354)

Co-authored-by: Eduard Tudenhoefner <[email protected]>
apache · Oct 25, 2024 · 7ad11b2 · 7ad11b2
1 parent 9ecd97b
commit 7ad11b2
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 3 deletions.
diff --git a/core/src/main/java/org/apache/iceberg/SnapshotParser.java b/core/src/main/java/org/apache/iceberg/SnapshotParser.java
@@ -129,13 +129,12 @@ static Snapshot fromJson(JsonNode node) {
           "Cannot parse summary from non-object value: %s",
           sNode);
 
+      operation = JsonUtil.getString(OPERATION, sNode);
       ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
       Iterator<String> fields = sNode.fieldNames();
       while (fields.hasNext()) {
         String field = fields.next();
-        if (field.equals(OPERATION)) {
-          operation = JsonUtil.getString(OPERATION, sNode);
-        } else {
+        if (!field.equals(OPERATION)) {
           builder.put(field, JsonUtil.getString(field, sNode));
         }
       }

diff --git a/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java b/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java
@@ -20,11 +20,16 @@
 
 import static org.apache.iceberg.Files.localInput;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.Map;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
 import org.junit.jupiter.api.Test;
@@ -35,6 +40,56 @@ public class TestSnapshotJson {
 
   public TableOperations ops = new LocalTableOperations(temp);
 
+  @Test
+  public void testToJsonWithoutOperation() throws IOException {
+    int snapshotId = 23;
+    Long parentId = null;
+    String manifestList = createManifestListWithManifestFiles(snapshotId, parentId);
+
+    Snapshot expected =
+        new BaseSnapshot(
+            0, snapshotId, parentId, System.currentTimeMillis(), null, null, 1, manifestList);
+    String json = SnapshotParser.toJson(expected);
+
+    // Assert that summary field is not present in the JSON
+    assertThat(new ObjectMapper().readTree(json)).anyMatch(node -> !node.has("summary"));
+  }
+
+  @Test
+  public void testToJsonWithOperation() throws IOException {
+    long parentId = 1;
+    long id = 2;
+
+    String manifestList = createManifestListWithManifestFiles(id, parentId);
+
+    Snapshot expected =
+        new BaseSnapshot(
+            0,
+            id,
+            parentId,
+            System.currentTimeMillis(),
+            DataOperations.REPLACE,
+            ImmutableMap.of("files-added", "4", "files-deleted", "100"),
+            3,
+            manifestList);
+    Map<String, String> expectedSummary =
+        ImmutableMap.<String, String>builder()
+            .putAll(expected.summary())
+            .put("operation", expected.operation()) // operation should be part of the summary
+            .build();
+
+    String json = SnapshotParser.toJson(expected);
+    ObjectMapper objectMapper = new ObjectMapper();
+    JsonNode jsonNode = objectMapper.readTree(json);
+
+    assertThat(jsonNode.get("summary")).isNotNull();
+
+    Map<String, String> actualSummary =
+        objectMapper.convertValue(
+            jsonNode.get("summary"), new TypeReference<Map<String, String>>() {});
+    assertThat(actualSummary).isEqualTo(expectedSummary);
+  }
+
   @Test
   public void testJsonConversion() throws IOException {
     int snapshotId = 23;
@@ -159,6 +214,28 @@ public void testJsonConversionWithV1Manifests() {
     assertThat(snapshot.schemaId()).isEqualTo(expected.schemaId());
   }
 
+  @Test
+  public void testJsonConversionSummaryWithoutOperationFails() {
+    String json =
+        String.format(
+            "{\n"
+                + "  \"snapshot-id\" : 2,\n"
+                + "  \"parent-snapshot-id\" : 1,\n"
+                + "  \"timestamp-ms\" : %s,\n"
+                + "  \"summary\" : {\n"
+                + "    \"files-added\" : \"4\",\n"
+                + "    \"files-deleted\" : \"100\"\n"
+                + "  },\n"
+                + "  \"manifests\" : [ \"/tmp/manifest1.avro\", \"/tmp/manifest2.avro\" ],\n"
+                + "  \"schema-id\" : 3\n"
+                + "}",
+            System.currentTimeMillis());
+
+    assertThatThrownBy(() -> SnapshotParser.fromJson(json))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot parse missing string: operation");
+  }
+
   private String createManifestListWithManifestFiles(long snapshotId, Long parentSnapshotId)
       throws IOException {
     File manifestList = File.createTempFile("manifests", null, temp.toFile());