gianm commented on code in PR #12873:
URL: https://github.com/apache/druid/pull/12873#discussion_r940494331
##########
core/src/main/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMaker.java:
##########
@@ -157,12 +165,13 @@ private Object convertJsonNode(JsonNode val)
Map<String, Object> newMap = new LinkedHashMap<>();
for (Iterator<Map.Entry<String, JsonNode>> it = val.fields();
it.hasNext(); ) {
Map.Entry<String, JsonNode> entry = it.next();
- newMap.put(entry.getKey(), valueConversionFunction(entry.getValue()));
+ newMap.put(entry.getKey(), finalizeConversionForMap(entry.getValue()));
}
return newMap;
}
- return val;
+ // turn anything else into a string value so that we don't leak JsonNode
into rows
+ return val.asText();
Review Comment:
Javadoc for `asText` says:
> Method that will return a valid String representation of the container
value, if the node is a value node (method isValueNode returns true), otherwise
empty String.
Since we're checking for boolean, number, null, and string already, I don't
think there are any cases here where `val.asText()` will return anything other
than an empty string. Perhaps we should return `null` here, instead, then?
##########
core/src/main/java/org/apache/druid/java/util/common/parsers/JSONFlattenerMaker.java:
##########
@@ -143,11 +143,19 @@ private Object convertJsonNode(JsonNode val)
return charsetFix(val.asText());
}
+ if (val.isBoolean()) {
+ return val.asBoolean();
+ }
+
+ if (val.isBinary() && val instanceof BinaryNode) {
+ return ((BinaryNode) val).binaryValue();
Review Comment:
Hmm. When does this happen? Javadocs for `isBinary` say:
> Method that can be used to check if this node represents binary data
(Base64 encoded). Although this will be externally written as JSON String
value, isTextual will return false if this method returns true.
Will this ever get generated from raw JSON? Does Jackson have some code that
detects stuff that looks like Base64 and then decodes it? If so: that seems
weird to me: IMO we should return the Base64 string, not the decoded `byte[]`
here. If people want to decode it then we should provide a `base64_decode`
function.
--
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]
