Jackie-Jiang commented on code in PR #18756:
URL: https://github.com/apache/pinot/pull/18756#discussion_r4253123214
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java:
##########
@@ -783,6 +792,150 @@ public static List<Map<String, String>> flatten(String
jsonString, JsonIndexConf
}
}
+ /// Flattens an already-parsed JSON value ({@link Map} / {@link List} /
{@link JsonNode}) for the JSON index, avoiding
+ /// the string tokenization that {@link #flatten(String, JsonIndexConfig)}
performs. Used by the realtime JSON index
+ /// when the source value is already a parsed object (e.g. cached on the
`GenericRow` before it is serialized to a
+ /// string for the forward index), so the document is parsed once at
ingestion instead of being serialized and
+ /// re-parsed here. The result matches flattening the forward value: Maps
use {@link MapUtils#toString(Map)}, while
+ /// Lists and JsonNodes use {@link #objectToString(Object)}. A String input
uses the existing string path.
+ public static List<Map<String, String>> flattenParsed(Object jsonValue,
JsonIndexConfig jsonIndexConfig)
+ throws IOException {
+ if (jsonValue instanceof String) {
+ return flatten((String) jsonValue, jsonIndexConfig);
+ }
+ if (!canFlattenParsedValue(jsonValue)) {
+ // Ingestion excludes these values from the cache, so its index reuses
the existing forward string.
+ return flatten(jsonValue instanceof Map ? MapUtils.toString((Map<String,
Object>) jsonValue)
+ : objectToString(jsonValue), jsonIndexConfig);
+ }
+ JsonNode jsonNode = jsonValue instanceof JsonNode ? (JsonNode) jsonValue
+ : (jsonValue instanceof Map ? PARSED_JSON_MAP_MAPPER :
PARSED_JSON_MAPPER).valueToTree(jsonValue);
+ int classification = classifyForFlatten(jsonNode);
Review Comment:
[P3] Reuse the JsonNode classification instead of traversing twice
For a JsonNode input, canFlattenParsedValue() at line 806 already calls
classifyForFlatten() on the entire tree. This line immediately repeats the same
classification. Text-derived cached JSON therefore incurs two identical
full-document traversals per row; inputs already supplied as JsonNode also
undergo the earlier transformer preflight.
Please compute and reuse the classification for JsonNode inputs so the
supported path avoids this redundant walk. The duplication is clear from the
call path; its throughput impact has not been measured.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java:
##########
@@ -783,6 +792,150 @@ public static List<Map<String, String>> flatten(String
jsonString, JsonIndexConf
}
}
+ /// Flattens an already-parsed JSON value ({@link Map} / {@link List} /
{@link JsonNode}) for the JSON index, avoiding
+ /// the string tokenization that {@link #flatten(String, JsonIndexConfig)}
performs. Used by the realtime JSON index
+ /// when the source value is already a parsed object (e.g. cached on the
`GenericRow` before it is serialized to a
+ /// string for the forward index), so the document is parsed once at
ingestion instead of being serialized and
+ /// re-parsed here. The result matches flattening the forward value: Maps
use {@link MapUtils#toString(Map)}, while
+ /// Lists and JsonNodes use {@link #objectToString(Object)}. A String input
uses the existing string path.
+ public static List<Map<String, String>> flattenParsed(Object jsonValue,
JsonIndexConfig jsonIndexConfig)
+ throws IOException {
+ if (jsonValue instanceof String) {
+ return flatten((String) jsonValue, jsonIndexConfig);
+ }
+ if (!canFlattenParsedValue(jsonValue)) {
+ // Ingestion excludes these values from the cache, so its index reuses
the existing forward string.
+ return flatten(jsonValue instanceof Map ? MapUtils.toString((Map<String,
Object>) jsonValue)
+ : objectToString(jsonValue), jsonIndexConfig);
+ }
+ JsonNode jsonNode = jsonValue instanceof JsonNode ? (JsonNode) jsonValue
+ : (jsonValue instanceof Map ? PARSED_JSON_MAP_MAPPER :
PARSED_JSON_MAPPER).valueToTree(jsonValue);
Review Comment:
[P1] Preserve JSON parser limits on the cached path
A supported Avro Map containing "ok":"v" and a 50,001-character String key
passes canFlattenParsedValue() and valueToTree(), but parsing its serialized
forward value throws StreamConstraintsException because Jackson's default
name-length limit is 50,000. JSON fields default to NO_ACTION for length
sanitization, so the parsed cache remains eligible. With skipInvalidJson=true,
the consuming index matches ok='v', while the immutable index rebuilt from the
same forward value skips the entire document. This changes query results after
sealing.
Please reject values exceeding the existing reader constraints from the
cache and reuse the forward-string path. Add regression coverage comparing
cached and immutable indexing with skipInvalidJson enabled. A bounded
reproduction against Jackson 2.22.3 confirms that serialization/valueToTree
succeed while readTree rejects this input.
--
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]
