This is an automated email from the ASF dual-hosted git repository.
fokko pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/master by this push:
new db7e09e54a Core: Improve collection handling in JsonUtil (#6051)
db7e09e54a is described below
commit db7e09e54ac7f4c48a5fa906c6e54b838e555ef7
Author: Eduard Tudenhöfner <[email protected]>
AuthorDate: Thu Nov 3 09:25:40 2022 +0100
Core: Improve collection handling in JsonUtil (#6051)
This aligns all collection methods to show the same error message and
behave the same with null/empty/invalid json values.
Additionally, this also adds the property name of the collection when
showing an error message
---
.../java/org/apache/iceberg/util/JsonUtil.java | 26 ++-
.../iceberg/metrics/TestScanReportParser.java | 4 +-
.../iceberg/puffin/TestFileMetadataParser.java | 2 +-
.../java/org/apache/iceberg/util/TestJsonUtil.java | 214 +++++++++++++++++++++
4 files changed, 238 insertions(+), 8 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/util/JsonUtil.java
b/core/src/main/java/org/apache/iceberg/util/JsonUtil.java
index 5d39c0cea6..6990464e62 100644
--- a/core/src/main/java/org/apache/iceberg/util/JsonUtil.java
+++ b/core/src/main/java/org/apache/iceberg/util/JsonUtil.java
@@ -212,8 +212,7 @@ public class JsonUtil {
}
public static Set<String> getStringSet(String property, JsonNode node) {
- Preconditions.checkArgument(
- node.hasNonNull(property), "Cannot parse missing set: %s", property);
+ Preconditions.checkArgument(node.has(property), "Cannot parse missing set:
%s", property);
return ImmutableSet.<String>builder()
.addAll(new JsonStringArrayIterator(property, node))
@@ -231,6 +230,7 @@ public class JsonUtil {
}
public static List<Integer> getIntegerList(String property, JsonNode node) {
+ Preconditions.checkArgument(node.has(property), "Cannot parse missing
list: %s", property);
return ImmutableList.<Integer>builder()
.addAll(new JsonIntegerArrayIterator(property, node))
.build();
@@ -245,6 +245,7 @@ public class JsonUtil {
}
public static Set<Integer> getIntegerSet(String property, JsonNode node) {
+ Preconditions.checkArgument(node.has(property), "Cannot parse missing set:
%s", property);
return ImmutableSet.<Integer>builder()
.addAll(new JsonIntegerArrayIterator(property, node))
.build();
@@ -255,6 +256,11 @@ public class JsonUtil {
return null;
}
+ return getLongSet(property, node);
+ }
+
+ public static Set<Long> getLongSet(String property, JsonNode node) {
+ Preconditions.checkArgument(node.has(property), "Cannot parse missing set:
%s", property);
return ImmutableSet.<Long>builder().addAll(new
JsonLongArrayIterator(property, node)).build();
}
@@ -304,9 +310,11 @@ public class JsonUtil {
}
static class JsonStringArrayIterator extends JsonArrayIterator<String> {
+ private final String property;
JsonStringArrayIterator(String property, JsonNode node) {
super(property, node);
+ this.property = property;
}
@Override
@@ -317,14 +325,19 @@ public class JsonUtil {
@Override
void validate(JsonNode element) {
Preconditions.checkArgument(
- element.isTextual(), "Cannot parse string from non-text value: %s",
element);
+ element.isTextual(),
+ "Cannot parse string from non-text value in %s: %s",
+ property,
+ element);
}
}
static class JsonIntegerArrayIterator extends JsonArrayIterator<Integer> {
+ private final String property;
JsonIntegerArrayIterator(String property, JsonNode node) {
super(property, node);
+ this.property = property;
}
@Override
@@ -335,14 +348,16 @@ public class JsonUtil {
@Override
void validate(JsonNode element) {
Preconditions.checkArgument(
- element.isInt(), "Cannot parse integer from non-int value: %s",
element);
+ element.isInt(), "Cannot parse integer from non-int value in %s:
%s", property, element);
}
}
static class JsonLongArrayIterator extends JsonArrayIterator<Long> {
+ private final String property;
JsonLongArrayIterator(String property, JsonNode node) {
super(property, node);
+ this.property = property;
}
@Override
@@ -354,7 +369,8 @@ public class JsonUtil {
void validate(JsonNode element) {
Preconditions.checkArgument(
element.isIntegralNumber() && element.canConvertToLong(),
- "Cannot parse long from non-long value: %s",
+ "Cannot parse long from non-long value in %s: %s",
+ property,
element);
}
}
diff --git
a/core/src/test/java/org/apache/iceberg/metrics/TestScanReportParser.java
b/core/src/test/java/org/apache/iceberg/metrics/TestScanReportParser.java
index 9bdb6c9f49..13130c9cfc 100644
--- a/core/src/test/java/org/apache/iceberg/metrics/TestScanReportParser.java
+++ b/core/src/test/java/org/apache/iceberg/metrics/TestScanReportParser.java
@@ -162,14 +162,14 @@ public class TestScanReportParser {
ScanReportParser.fromJson(
"{\"table-name\":\"roundTripTableName\",\"snapshot-id\":23,\"filter\":true,\"schema-id\":23,\"projected-field-ids\":
[\"1\"],\"metrics\":{}}"))
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot parse integer from non-int value: \"1\"");
+ .hasMessage("Cannot parse integer from non-int value in
projected-field-ids: \"1\"");
Assertions.assertThatThrownBy(
() ->
ScanReportParser.fromJson(
"{\"table-name\":\"roundTripTableName\",\"snapshot-id\":23,\"filter\":true,\"schema-id\":23,\"projected-field-ids\":
[1],\"projected-field-names\": [1],\"metrics\":{}}"))
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot parse string from non-text value: 1");
+ .hasMessage("Cannot parse string from non-text value in
projected-field-names: 1");
}
@Test
diff --git
a/core/src/test/java/org/apache/iceberg/puffin/TestFileMetadataParser.java
b/core/src/test/java/org/apache/iceberg/puffin/TestFileMetadataParser.java
index 81192be627..c105283753 100644
--- a/core/src/test/java/org/apache/iceberg/puffin/TestFileMetadataParser.java
+++ b/core/src/test/java/org/apache/iceberg/puffin/TestFileMetadataParser.java
@@ -173,7 +173,7 @@ public class TestFileMetadataParser {
+ " } ]\n"
+ "}"))
.isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot parse integer from non-int value: 2147483648");
+ .hasMessage("Cannot parse integer from non-int value in fields:
2147483648");
}
private void testJsonSerialization(FileMetadata fileMetadata, String json) {
diff --git a/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java
b/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java
index 6fc6c6a79b..7b3f3be8de 100644
--- a/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java
+++ b/core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java
@@ -19,6 +19,8 @@
package org.apache.iceberg.util;
import com.fasterxml.jackson.core.JsonProcessingException;
+import java.util.Arrays;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.assertj.core.api.Assertions;
import org.junit.Test;
@@ -189,4 +191,216 @@ public class TestJsonUtil {
Assertions.assertThat(JsonUtil.getBool("x",
JsonUtil.mapper().readTree("{\"x\": false}")))
.isFalse();
}
+
+ @Test
+ public void getIntegerList() throws JsonProcessingException {
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getIntegerList("items",
JsonUtil.mapper().readTree("{}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse missing list: items");
+
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getIntegerList("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse from non-array value: items: null");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getIntegerList(
+ "items", JsonUtil.mapper().readTree("{\"items\": [13,
\"23\"]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse integer from non-int value in items:
\"23\"");
+
+ Assertions.assertThat(
+ JsonUtil.getIntegerList("items",
JsonUtil.mapper().readTree("{\"items\": [23, 45]}")))
+ .isEqualTo(Arrays.asList(23, 45));
+ }
+
+ @Test
+ public void getIntegerSet() throws JsonProcessingException {
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getIntegerSet("items",
JsonUtil.mapper().readTree("{}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse missing set: items");
+
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getIntegerSet("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse from non-array value: items: null");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getIntegerSet(
+ "items", JsonUtil.mapper().readTree("{\"items\": [13,
\"23\"]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse integer from non-int value in items:
\"23\"");
+
+ Assertions.assertThat(
+ JsonUtil.getIntegerSet("items",
JsonUtil.mapper().readTree("{\"items\": [23, 45]}")))
+ .containsExactlyElementsOf(Arrays.asList(23, 45));
+ }
+
+ @Test
+ public void getIntegerSetOrNull() throws JsonProcessingException {
+ Assertions.assertThat(JsonUtil.getIntegerSetOrNull("items",
JsonUtil.mapper().readTree("{}")))
+ .isNull();
+
+ Assertions.assertThat(
+ JsonUtil.getIntegerSetOrNull("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isNull();
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getIntegerSetOrNull(
+ "items", JsonUtil.mapper().readTree("{\"items\": [13,
\"23\"]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse integer from non-int value in items:
\"23\"");
+
+ Assertions.assertThat(
+ JsonUtil.getIntegerSetOrNull(
+ "items", JsonUtil.mapper().readTree("{\"items\": [23, 45]}")))
+ .containsExactlyElementsOf(Arrays.asList(23, 45));
+ }
+
+ @Test
+ public void getLongSet() throws JsonProcessingException {
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getLongSet("items",
JsonUtil.mapper().readTree("{}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse missing set: items");
+
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getLongSet("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse from non-array value: items: null");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getLongSet(
+ "items", JsonUtil.mapper().readTree("{\"items\": [13,
\"23\"]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse long from non-long value in items: \"23\"");
+
+ Assertions.assertThat(
+ JsonUtil.getLongSet("items",
JsonUtil.mapper().readTree("{\"items\": [23, 45]}")))
+ .containsExactlyElementsOf(Arrays.asList(23L, 45L));
+ }
+
+ @Test
+ public void getLongSetOrNull() throws JsonProcessingException {
+ Assertions.assertThat(JsonUtil.getLongSetOrNull("items",
JsonUtil.mapper().readTree("{}")))
+ .isNull();
+
+ Assertions.assertThat(
+ JsonUtil.getLongSetOrNull("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isNull();
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getLongSetOrNull(
+ "items", JsonUtil.mapper().readTree("{\"items\": [13,
\"23\"]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse long from non-long value in items: \"23\"");
+
+ Assertions.assertThat(
+ JsonUtil.getLongSetOrNull("items",
JsonUtil.mapper().readTree("{\"items\": [23, 45]}")))
+ .containsExactlyElementsOf(Arrays.asList(23L, 45L));
+ }
+
+ @Test
+ public void getStringList() throws JsonProcessingException {
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getStringList("items",
JsonUtil.mapper().readTree("{}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse missing list: items");
+
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getStringList("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse from non-array value: items: null");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getStringList(
+ "items", JsonUtil.mapper().readTree("{\"items\": [\"23\",
45]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse string from non-text value in items: 45");
+
+ Assertions.assertThat(
+ JsonUtil.getStringList(
+ "items", JsonUtil.mapper().readTree("{\"items\": [\"23\",
\"45\"]}")))
+ .containsExactlyElementsOf(Arrays.asList("23", "45"));
+ }
+
+ @Test
+ public void getStringListOrNull() throws JsonProcessingException {
+ Assertions.assertThat(JsonUtil.getStringListOrNull("items",
JsonUtil.mapper().readTree("{}")))
+ .isNull();
+
+ Assertions.assertThat(
+ JsonUtil.getStringListOrNull("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isNull();
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getStringListOrNull(
+ "items", JsonUtil.mapper().readTree("{\"items\": [\"23\",
45]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse string from non-text value in items: 45");
+
+ Assertions.assertThat(
+ JsonUtil.getStringListOrNull(
+ "items", JsonUtil.mapper().readTree("{\"items\": [\"23\",
\"45\"]}")))
+ .containsExactlyElementsOf(Arrays.asList("23", "45"));
+ }
+
+ @Test
+ public void getStringSet() throws JsonProcessingException {
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getStringSet("items",
JsonUtil.mapper().readTree("{}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse missing set: items");
+
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getStringSet("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse from non-array value: items: null");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getStringSet(
+ "items", JsonUtil.mapper().readTree("{\"items\": [\"23\",
45]}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse string from non-text value in items: 45");
+
+ Assertions.assertThat(
+ JsonUtil.getStringSet(
+ "items", JsonUtil.mapper().readTree("{\"items\": [\"23\",
\"45\"]}")))
+ .containsExactlyElementsOf(Arrays.asList("23", "45"));
+ }
+
+ @Test
+ public void getStringMap() throws JsonProcessingException {
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getStringMap("items",
JsonUtil.mapper().readTree("{}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse missing map: items");
+
+ Assertions.assertThatThrownBy(
+ () -> JsonUtil.getStringMap("items",
JsonUtil.mapper().readTree("{\"items\": null}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse from non-object value: items: null");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ JsonUtil.getStringMap(
+ "items", JsonUtil.mapper().readTree("{\"items\":
{\"a\":\"23\", \"b\":45}}")))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse to a string value: b: 45");
+
+ Assertions.assertThat(
+ JsonUtil.getStringMap(
+ "items", JsonUtil.mapper().readTree("{\"items\":
{\"a\":\"23\", \"b\":\"45\"}}")))
+ .isEqualTo(ImmutableMap.of("a", "23", "b", "45"));
+ }
}
