This is an automated email from the ASF dual-hosted git repository.
russellspitzer 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 6c58f5bb7f Revert "Core: Snapshot `summary` map must have `operation`
key (#11354)" (#11409)
6c58f5bb7f is described below
commit 6c58f5bb7f32ec3b322f68f15c82a62a95267e77
Author: Kevin Liu <[email protected]>
AuthorDate: Mon Oct 28 13:32:19 2024 -0400
Revert "Core: Snapshot `summary` map must have `operation` key (#11354)"
(#11409)
This reverts commit 7ad11b2df1a266d29f9e4f6bb5b499cb68c0afb7.
---
.../java/org/apache/iceberg/SnapshotParser.java | 5 +-
.../java/org/apache/iceberg/TestSnapshotJson.java | 77 ----------------------
2 files changed, 3 insertions(+), 79 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/SnapshotParser.java
b/core/src/main/java/org/apache/iceberg/SnapshotParser.java
index 41b8e1499c..bc5ef60946 100644
--- a/core/src/main/java/org/apache/iceberg/SnapshotParser.java
+++ b/core/src/main/java/org/apache/iceberg/SnapshotParser.java
@@ -129,12 +129,13 @@ 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)) {
+ if (field.equals(OPERATION)) {
+ operation = JsonUtil.getString(OPERATION, sNode);
+ } else {
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 7fff5c5ddd..ee12390749 100644
--- a/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java
+++ b/core/src/test/java/org/apache/iceberg/TestSnapshotJson.java
@@ -20,16 +20,11 @@ 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;
@@ -40,56 +35,6 @@ 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;
@@ -214,28 +159,6 @@ 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());
