(iceberg) branch main updated: Core: Snapshot `summary` map must have `operation` key (#11354)

[amoghj]

This is an automated email from the ASF dual-hosted git repository.

amoghj pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git


The following commit(s) were added to refs/heads/main by this push:
     new 7ad11b2df1 Core: Snapshot `summary` map must have `operation` key 
(#11354)
7ad11b2df1 is described below

commit 7ad11b2df1a266d29f9e4f6bb5b499cb68c0afb7
Author: Kevin Liu <kevinjq...@users.noreply.github.com>
AuthorDate: Fri Oct 25 17:23:44 2024 -0400

    Core: Snapshot `summary` map must have `operation` key (#11354)
    
    Co-authored-by: Eduard Tudenhoefner <etudenhoef...@gmail.com>
---
 .../java/org/apache/iceberg/SnapshotParser.java    |  5 +-
 .../java/org/apache/iceberg/TestSnapshotJson.java  | 77 ++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/org/apache/iceberg/SnapshotParser.java 
b/core/src/main/java/org/apache/iceberg/SnapshotParser.java
index bc5ef60946..41b8e1499c 100644
--- a/core/src/main/java/org/apache/iceberg/SnapshotParser.java
+++ b/core/src/main/java/org/apache/iceberg/SnapshotParser.java
@@ -129,13 +129,12 @@ public class SnapshotParser {
           "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
index ee12390749..7fff5c5ddd 100644
--- a/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java
+++ b/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java
@@ -20,11 +20,16 @@ package org.apache.iceberg;
 
 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 class TestSnapshotJson {
     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());