stevenzwu commented on code in PR #16586:
URL: https://github.com/apache/iceberg/pull/16586#discussion_r3315892228
##########
core/src/main/java/org/apache/iceberg/util/JsonUtil.java:
##########
@@ -272,7 +272,10 @@ public static String[] getStringArray(JsonNode node) {
ArrayNode arrayNode = (ArrayNode) node;
String[] arr = new String[arrayNode.size()];
for (int i = 0; i < arr.length; i++) {
- arr[i] = arrayNode.get(i).asText();
+ JsonNode element = arrayNode.get(i);
+ Preconditions.checkArgument(
+ element.isTextual(), "Cannot parse string from non-text value: %s",
element);
Review Comment:
Fair point that `getStringList`/`getStringSet` carry the property name and
this one does not. Using `node` (the full array) in the format string is noisy
on large arrays and does not actually identify the field, though.
A cleaner option: add an overloaded `getStringArray(String property,
JsonNode node)` mirroring `getStringList(String, JsonNode)`, and migrate the
two callers that already have a property name in scope:
- `ViewVersionParser` has `DEFAULT_NAMESPACE`
- `RemoteSignRequestParser` has the header key
`RESTSerializers.NamespaceDeserializer` deserializes a top-level `Namespace`
with no field name in scope, so it would stay on the existing 1-arg overload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
