address Eduard's initial comments

apache · Aug 14, 2024 · a294130 · a294130
1 parent 176ea2b
commit a294130
Show file tree

Hide file tree

Showing 9 changed files with 141 additions and 29 deletions.
diff --git a/core/src/main/java/org/apache/iceberg/AllManifestsTableTaskParser.java b/core/src/main/java/org/apache/iceberg/AllManifestsTableTaskParser.java
@@ -80,7 +80,7 @@ static AllManifestsTable.ManifestListReadTask fromJson(JsonNode jsonNode) {
     FileIO fileIO = FileIOParser.fromJson(JsonUtil.get(FILE_IO, jsonNode), null);
     Schema schema = SchemaParser.fromJson(JsonUtil.get(SCHEMA, jsonNode));
 
-    JsonNode specsArray = jsonNode.get(SPECS);
+    JsonNode specsArray = JsonUtil.get(SPECS, jsonNode);
     Preconditions.checkArgument(
         specsArray.isArray(), "Invalid JSON node for partition specs: non-array (%s)", specsArray);
 

diff --git a/core/src/main/java/org/apache/iceberg/BaseEntriesTable.java b/core/src/main/java/org/apache/iceberg/BaseEntriesTable.java
@@ -276,7 +276,8 @@ static class ManifestReadTask extends BaseFileScanTask implements DataTask {
     private final ManifestFile manifest;
     private final Map<Integer, PartitionSpec> specsById;
 
-    ManifestReadTask(Table table, ManifestFile manifest, Schema projection, Expression filter) {
+    private ManifestReadTask(
+        Table table, ManifestFile manifest, Schema projection, Expression filter) {
       this(table.schema(), table.io(), table.specs(), manifest, projection, filter);
     }
 

diff --git a/core/src/main/java/org/apache/iceberg/BaseFilesTable.java b/core/src/main/java/org/apache/iceberg/BaseFilesTable.java
@@ -134,7 +134,8 @@ static class ManifestReadTask extends BaseFileScanTask implements DataTask {
     private final Schema dataTableSchema;
     private final Schema projection;
 
-    ManifestReadTask(Table table, ManifestFile manifest, Schema projection, Expression filter) {
+    private ManifestReadTask(
+        Table table, ManifestFile manifest, Schema projection, Expression filter) {
       this(table.schema(), table.io(), table.specs(), manifest, projection, filter);
     }
 

diff --git a/core/src/main/java/org/apache/iceberg/FilesTableTaskParser.java b/core/src/main/java/org/apache/iceberg/FilesTableTaskParser.java
@@ -43,7 +43,7 @@ private FilesTableTaskParser() {}
 
   static void toJson(BaseFilesTable.ManifestReadTask task, JsonGenerator generator)
       throws IOException {
-    Preconditions.checkArgument(task != null, "Invalid manifest task: null");
+    Preconditions.checkArgument(task != null, "Invalid files task: null");
     Preconditions.checkArgument(generator != null, "Invalid JSON generator: null");
 
     generator.writeFieldName(SCHEMA);
@@ -70,16 +70,16 @@ static void toJson(BaseFilesTable.ManifestReadTask task, JsonGenerator generator
   }
 
   static BaseFilesTable.ManifestReadTask fromJson(JsonNode jsonNode) {
-    Preconditions.checkArgument(jsonNode != null, "Invalid JSON node for manifest task: null");
+    Preconditions.checkArgument(jsonNode != null, "Invalid JSON node for files task: null");
     Preconditions.checkArgument(
-        jsonNode.isObject(), "Invalid JSON node for manifest task: non-object (%s)", jsonNode);
+        jsonNode.isObject(), "Invalid JSON node for files task: non-object (%s)", jsonNode);
 
     Schema dataTableSchema = SchemaParser.fromJson(JsonUtil.get(SCHEMA, jsonNode));
     Schema projection = SchemaParser.fromJson(JsonUtil.get(PROJECTION, jsonNode));
 
     FileIO fileIO = FileIOParser.fromJson(JsonUtil.get(FILE_IO, jsonNode), null);
 
-    JsonNode specsArray = jsonNode.get(SPECS);
+    JsonNode specsArray = JsonUtil.get(SPECS, jsonNode);
     Preconditions.checkArgument(
         specsArray.isArray(), "Invalid JSON node for partition specs: non-array (%s)", specsArray);
 

diff --git a/core/src/main/java/org/apache/iceberg/ManifestEntriesTableTaskParser.java b/core/src/main/java/org/apache/iceberg/ManifestEntriesTableTaskParser.java
@@ -77,7 +77,7 @@ static BaseEntriesTable.ManifestReadTask fromJson(JsonNode jsonNode) {
     Schema dataTableSchema = SchemaParser.fromJson(JsonUtil.get(SCHEMA, jsonNode));
     FileIO fileIO = FileIOParser.fromJson(JsonUtil.get(FILE_IO, jsonNode), null);
 
-    JsonNode specsArray = jsonNode.get(SPECS);
+    JsonNode specsArray = JsonUtil.get(SPECS, jsonNode);
     Preconditions.checkArgument(
         specsArray.isArray(), "Invalid JSON node for partition specs: non-array (%s)", specsArray);
     ImmutableList.Builder<PartitionSpec> specsBuilder = ImmutableList.builder();

diff --git a/core/src/main/java/org/apache/iceberg/ManifestFileParser.java b/core/src/main/java/org/apache/iceberg/ManifestFileParser.java
@@ -25,8 +25,7 @@
 import java.util.List;
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
-import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
-import org.apache.iceberg.util.ByteBuffers;
+import org.apache.iceberg.types.Types;
 import org.apache.iceberg.util.JsonUtil;
 
 class ManifestFileParser {
@@ -103,9 +102,8 @@ static void toJson(ManifestFile manifestFile, JsonGenerator generator) throws IO
     }
 
     if (manifestFile.keyMetadata() != null) {
-      generator.writeStringField(
-          KEY_METADATA,
-          BaseEncoding.base16().encode(ByteBuffers.toByteArray(manifestFile.keyMetadata())));
+      generator.writeFieldName(KEY_METADATA);
+      SingleValueParser.toJson(DataFile.KEY_METADATA.type(), manifestFile.keyMetadata(), generator);
     }
 
     generator.writeEndObject();
@@ -165,7 +163,7 @@ static ManifestFile fromJson(JsonNode jsonNode) {
 
     List<ManifestFile.PartitionFieldSummary> partitionFieldSummaries = null;
     if (jsonNode.has(PARTITION_FIELD_SUMMARY)) {
-      JsonNode summaryArray = jsonNode.get(PARTITION_FIELD_SUMMARY);
+      JsonNode summaryArray = JsonUtil.get(PARTITION_FIELD_SUMMARY, jsonNode);
       Preconditions.checkArgument(
           summaryArray.isArray(),
           "Invalid JSON node for partition field summaries: non-array (%s)",
@@ -181,11 +179,7 @@ static ManifestFile fromJson(JsonNode jsonNode) {
       partitionFieldSummaries = builder.build();
     }
 
-    ByteBuffer keyMetadata = null;
-    if (jsonNode.has(KEY_METADATA)) {
-      String hexStr = JsonUtil.getString(KEY_METADATA, jsonNode);
-      keyMetadata = ByteBuffer.wrap(BaseEncoding.base16().decode(hexStr));
-    }
+    ByteBuffer keyMetadata = JsonUtil.getByteBufferOrNull(KEY_METADATA, jsonNode);
 
     return new GenericManifestFile(
         path,
@@ -227,15 +221,13 @@ static void toJson(ManifestFile.PartitionFieldSummary summary, JsonGenerator gen
       }
 
       if (summary.lowerBound() != null) {
-        generator.writeStringField(
-            LOWER_BOUND,
-            BaseEncoding.base16().encode(ByteBuffers.toByteArray(summary.lowerBound())));
+        generator.writeFieldName(LOWER_BOUND);
+        SingleValueParser.toJson(Types.BinaryType.get(), summary.lowerBound(), generator);
       }
 
       if (summary.upperBound() != null) {
-        generator.writeStringField(
-            UPPER_BOUND,
-            BaseEncoding.base16().encode(ByteBuffers.toByteArray(summary.upperBound())));
+        generator.writeFieldName(UPPER_BOUND);
+        SingleValueParser.toJson(Types.BinaryType.get(), summary.upperBound(), generator);
       }
 
       generator.writeEndObject();
@@ -257,14 +249,16 @@ static ManifestFile.PartitionFieldSummary fromJson(JsonNode jsonNode) {
 
       ByteBuffer lowerBound = null;
       if (jsonNode.has(LOWER_BOUND)) {
-        String hexStr = JsonUtil.getString(LOWER_BOUND, jsonNode);
-        lowerBound = ByteBuffer.wrap(BaseEncoding.base16().decode(hexStr));
+        lowerBound =
+            (ByteBuffer)
+                SingleValueParser.fromJson(Types.BinaryType.get(), jsonNode.get(LOWER_BOUND));
       }
 
       ByteBuffer upperBound = null;
       if (jsonNode.has(UPPER_BOUND)) {
-        String hexStr = JsonUtil.getString(UPPER_BOUND, jsonNode);
-        upperBound = ByteBuffer.wrap(BaseEncoding.base16().decode(hexStr));
+        upperBound =
+            (ByteBuffer)
+                SingleValueParser.fromJson(Types.BinaryType.get(), jsonNode.get(UPPER_BOUND));
       }
 
       if (containsNaN != null) {

diff --git a/core/src/test/java/org/apache/iceberg/TestAllManifestsTableTaskParser.java b/core/src/test/java/org/apache/iceberg/TestAllManifestsTableTaskParser.java
@@ -19,16 +19,55 @@
 package org.apache.iceberg;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.StringWriter;
 import java.util.Arrays;
 import java.util.Map;
 import org.apache.iceberg.expressions.Expressions;
 import org.apache.iceberg.hadoop.HadoopFileIO;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.JsonUtil;
 import org.apache.iceberg.util.PartitionUtil;
 import org.junit.jupiter.api.Test;
 
 public class TestAllManifestsTableTaskParser {
+  @Test
+  public void nullCheck() throws Exception {
+    StringWriter writer = new StringWriter();
+    JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
+
+    assertThatThrownBy(() -> AllManifestsTableTaskParser.toJson(null, generator))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid manifest task: null");
+
+    assertThatThrownBy(() -> AllManifestsTableTaskParser.toJson(createTask(), null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid JSON generator: null");
+
+    assertThatThrownBy(() -> AllManifestsTableTaskParser.fromJson(null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid JSON node for manifest task: null");
+  }
+
+  @Test
+  public void invalidJsonNode() throws Exception {
+    String jsonStr = "{\"str\":\"1\", \"arr\":[]}";
+    ObjectMapper mapper = new ObjectMapper();
+    JsonNode rootNode = mapper.reader().readTree(jsonStr);
+
+    assertThatThrownBy(() -> AllManifestsTableTaskParser.fromJson(rootNode.get("str")))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid JSON node for manifest task: non-object ");
+
+    assertThatThrownBy(() -> AllManifestsTableTaskParser.fromJson(rootNode.get("arr")))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid JSON node for manifest task: non-object ");
+  }
+
   @Test
   public void testParser() {
     AllManifestsTable.ManifestListReadTask task = createTask();

diff --git a/core/src/test/java/org/apache/iceberg/TestFilesTableTaskParser.java b/core/src/test/java/org/apache/iceberg/TestFilesTableTaskParser.java
@@ -19,16 +19,55 @@
 package org.apache.iceberg;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.StringWriter;
 import java.util.Arrays;
 import java.util.Map;
 import org.apache.iceberg.expressions.Expressions;
 import org.apache.iceberg.hadoop.HadoopFileIO;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.JsonUtil;
 import org.apache.iceberg.util.PartitionUtil;
 import org.junit.jupiter.api.Test;
 
 public class TestFilesTableTaskParser {
+  @Test
+  public void nullCheck() throws Exception {
+    StringWriter writer = new StringWriter();
+    JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
+
+    assertThatThrownBy(() -> FilesTableTaskParser.toJson(null, generator))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid files task: null");
+
+    assertThatThrownBy(() -> FilesTableTaskParser.toJson(createTask(), null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid JSON generator: null");
+
+    assertThatThrownBy(() -> FilesTableTaskParser.fromJson(null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid JSON node for files task: null");
+  }
+
+  @Test
+  public void invalidJsonNode() throws Exception {
+    String jsonStr = "{\"str\":\"1\", \"arr\":[]}";
+    ObjectMapper mapper = new ObjectMapper();
+    JsonNode rootNode = mapper.reader().readTree(jsonStr);
+
+    assertThatThrownBy(() -> FilesTableTaskParser.fromJson(rootNode.get("str")))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid JSON node for files task: non-object ");
+
+    assertThatThrownBy(() -> FilesTableTaskParser.fromJson(rootNode.get("arr")))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid JSON node for files task: non-object ");
+  }
+
   @Test
   public void testParser() {
     BaseFilesTable.ManifestReadTask task = createTask();

diff --git a/core/src/test/java/org/apache/iceberg/TestManifestFileParser.java b/core/src/test/java/org/apache/iceberg/TestManifestFileParser.java
@@ -19,16 +19,54 @@
 package org.apache.iceberg;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.StringWriter;
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.List;
 import org.apache.iceberg.types.Conversions;
 import org.apache.iceberg.types.Types;
 import org.apache.iceberg.util.JsonUtil;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestTemplate;
 
 public class TestManifestFileParser extends TestBase {
+  @Test
+  public void nullCheck() throws Exception {
+    StringWriter writer = new StringWriter();
+    JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
+
+    assertThatThrownBy(() -> ManifestFileParser.toJson(null, generator))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid manifest file: null");
+
+    assertThatThrownBy(() -> ManifestFileParser.toJson(createManifestFile(), null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid JSON generator: null");
+
+    assertThatThrownBy(() -> ManifestFileParser.fromJson(null))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Invalid JSON node for manifest file: null");
+  }
+
+  @Test
+  public void invalidJsonNode() throws Exception {
+    String jsonStr = "{\"str\":\"1\", \"arr\":[]}";
+    ObjectMapper mapper = new ObjectMapper();
+    JsonNode rootNode = mapper.reader().readTree(jsonStr);
+
+    assertThatThrownBy(() -> ManifestFileParser.fromJson(rootNode.get("str")))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid JSON node for manifest file: non-object ");
+
+    assertThatThrownBy(() -> ManifestFileParser.fromJson(rootNode.get("arr")))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessageContaining("Invalid JSON node for manifest file: non-object ");
+  }
 
   @TestTemplate
   public void testParser() throws Exception {