Jackie-Jiang commented on code in PR #18756:
URL: https://github.com/apache/pinot/pull/18756#discussion_r4201800026
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java:
##########
@@ -783,6 +783,115 @@ 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 must match {@link #flatten(String,
JsonIndexConfig)} on the serialized form (both go
+ /// through `DEFAULT_MAPPER`). A {@link String} input is delegated to {@link
#flatten(String, JsonIndexConfig)}.
+ public static List<Map<String, String>> flattenParsed(Object jsonValue,
JsonIndexConfig jsonIndexConfig)
+ throws IOException {
+ if (jsonValue instanceof String) {
+ return flatten((String) jsonValue, jsonIndexConfig);
+ }
+ JsonNode jsonNode = jsonValue instanceof JsonNode ? (JsonNode) jsonValue :
DEFAULT_MAPPER.valueToTree(jsonValue);
Review Comment:
[P1] Preserve the original numeric representation when converting a Map
Jackson's default valueToTree() strips BigDecimal trailing zeros before
normalizeBigDecimalNodes() reparses the leaf. This is reachable through the
standard JSON reader, which converts integers beyond long range into
BigDecimal. For {"jsonCol":{"a":123456789012345678900}}, the forward value
retains the exact integer, but the cached tree contains
1.234567890123456789E+20, which is subsequently parsed as a double and indexed
as 1.2345678901234568E20. Exact JSON_MATCH equality therefore differs between
the consuming segment and the immutable index rebuilt from its forward values.
A smaller example is Map.of("a", new BigDecimal("2.0")): the indexed text
changes from "2.0" to "2".
Please preserve the original decimal representation when converting Map/List
inputs, or use the existing string path for affected values. Add a focused
regression without Float/binary leaves: the new mixed-Map test includes a
Float, forcing the entire document through fallback and masking this defect.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/DataTypeTransformer.java:
##########
@@ -131,8 +167,39 @@ public void transform(GenericRow record) {
&& value instanceof CharSequence) {
validateCanonicalUuidPrimaryKey(column, value.toString());
}
- value = DataTypeTransformerUtils.transformValue(column, value,
entry.getValue());
+ // For a JSON column with a JSON index, cache the parsed value so the
index flattens it directly instead of
+ // re-parsing the string the value is serialized into for the forward
index. The parsed value is cached AFTER
+ // the forward value is written (below), because putValue invalidates
the parsed-JSON cache: a later transformer
+ // that rewrites the value (e.g. sanitization trimming an over-length
JSON string) must not leave a stale node.
+ Object parsedToCache = null;
+ if (value != null && !_jsonCacheColumns.isEmpty() &&
_jsonCacheColumns.contains(column)) {
+ if (value instanceof Map || value instanceof List || value
instanceof JsonNode) {
+ // Already-parsed JSON: serialize it for the forward index (below)
and cache the parsed value for the index.
+ parsedToCache = value;
Review Comment:
[P1] Preserve the canonical Map ordering used by the forward value
This caches the original Map, but the conversion below serializes Map inputs
through MapUtils.toString(), which sorts keys. flattenParsed() instead uses an
unsorted mapper, and flattening overwrites entries when a dotted key and a
nested path produce the same flattened key. For a LinkedHashMap with a.b=2
inserted before a={b:1}, the existing sorted string path indexes .a.b as "2",
while the cached path indexes it as "1". JSON_MATCH on "$.a.b" can consequently
return different results before and after segment sealing.
Please preserve the canonical serialization order in the parsed path and its
fallback, or fall back for colliding paths. Regression coverage should compare
against the actual forward string produced by DataTypeTransformer/MapUtils,
rather than only JsonUtils.objectToString().
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java:
##########
@@ -783,6 +783,115 @@ 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 must match {@link #flatten(String,
JsonIndexConfig)} on the serialized form (both go
+ /// through `DEFAULT_MAPPER`). A {@link String} input is delegated to {@link
#flatten(String, JsonIndexConfig)}.
+ public static List<Map<String, String>> flattenParsed(Object jsonValue,
JsonIndexConfig jsonIndexConfig)
+ throws IOException {
+ if (jsonValue instanceof String) {
+ return flatten((String) jsonValue, jsonIndexConfig);
+ }
+ JsonNode jsonNode = jsonValue instanceof JsonNode ? (JsonNode) jsonValue :
DEFAULT_MAPPER.valueToTree(jsonValue);
+ int classification = classifyForFlatten(jsonNode);
+ if (classification == FLATTEN_UNSAFE) {
+ // A leaf renders differently than the string path (e.g. a Float or
byte[] from a non-JSON RecordReader). Fall
+ // back to serialize+reparse so the flattened records are byte-for-byte
identical. Rare: the JSON decoders never
+ // produce these types.
+ return flatten(objectToString(jsonValue), jsonIndexConfig);
Review Comment:
[P2] Reuse the existing forward-index string on fallback
For a document containing Float or byte[] leaves, DataTypeTransformer has
already serialized the document for the forward index. This path then builds a
complete tree with valueToTree(), classifies it, serializes the original
document again, and finally performs the original parse/flatten. Ordinary Avro
records/maps can contain these leaf types, so those rows retain all previous
work and add another tree conversion plus a redundant whole-document
serialization.
Please pass the already-produced canonical column string to the fallback, or
decline the optimization before doing the full tree conversion for unsupported
values. Include an Avro-style float/binary payload in performance coverage; the
current integer/string Map benchmark does not exercise this 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]
