xiangfu0 commented on code in PR #18756:
URL: https://github.com/apache/pinot/pull/18756#discussion_r4254300456
##########
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:
Fixed in a0b97e2df7. The JSON index now classifies each cached value against
the constraints of the reader `flatten(String)` parses with (`DEFAULT_READER`):
nesting depth, name length, string length, and integer digit count, with the
Jackson 2.22.3 boundaries checked on both sides (e.g. 50,000 vs 50,001-char
names). A value outside them indexes the forward string, which
`MutableSegmentImpl` now passes along with the cached value.
Two related gaps showed up while testing:
- Decimal leaves were re-parsed as bare tokens, and Jackson accepts a plain
decimal one digit over the number-length limit at the end of its input, so a
nested 1,001-digit decimal still diverged. Nested decimals near the limit are
now re-parsed inside a container, and a failure while converting the value
defers to the string path (which also covers decimals that the reader accepts
at an input buffer boundary in forward values over 32K chars).
- Jackson's hash-flooding defense (field names colliding in its name table)
is not replicated; the `flattenParsed` Javadoc documents that a crafted
document of that kind can still be indexed by the consuming segment.
`JsonIndexTest#testMutableJsonIndexCachedValueRespectsParserConstraints`
reproduces your scenario, plus an over-limit integer, an `ObjectNode`, an
over-limit decimal, and objects nested past the depth limit, with
`skipInvalidJson`, and compares the consuming index with the immutable index
built from the forward values.
`JsonUtilsTest#testFlattenParsedRespectsParserConstraints` covers both sides of
each limit. Both fail on the previous head.
##########
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:
Fixed in a0b97e2df7. Each cached value is now classified exactly once per
row, in the index, and that result drives both the direct-path decision and the
decimal normalization. Following your earlier P2 suggestion,
`DataTypeTransformer` no longer runs a preflight: it caches the parsed value,
and `MutableSegmentImpl` passes the forward value to `addParsed`, so a value
that cannot be flattened directly (Float/binary leaves, limit violations)
indexes that string without another serialization. This covers text-derived and
reader-supplied `JsonNode`s as well as Maps and Lists.
In the A/B benchmark in the PR description, every measured input is equal or
faster than the previous head (text-derived `JsonNode` rows 2,983 -> 2,903
ns/op, about 0.75x of master's string path).
--
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]
