Copilot commented on code in PR #18756:
URL: https://github.com/apache/pinot/pull/18756#discussion_r3409215258
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/DataTypeTransformer.java:
##########
@@ -69,6 +116,12 @@ public void transform(GenericRow record) {
String column = entry.getKey();
try {
Object value = record.getValue(column);
+ if (!_jsonCacheColumns.isEmpty() && _jsonCacheColumns.contains(column)
+ && (value instanceof Map || value instanceof List)) {
Review Comment:
The JSON parse-once cache currently only caches parsed values when the
incoming value is a Map or List. If a RecordReader provides a parsed JSON value
as a Jackson JsonNode (which JsonUtils.flattenParsed() explicitly supports), it
will not be cached and the JSON index will fall back to the slower re-parse
path. Consider including JsonNode in the cache predicate so all supported
parsed representations benefit.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/mutable/MutableJsonIndex.java:
##########
@@ -31,16 +32,34 @@ public interface MutableJsonIndex extends JsonIndexReader,
MutableIndex {
@Override
default void add(Object value, int dictId, int docId) {
try {
- if (value instanceof Map) {
- add(JsonUtils.objectToString(value));
+ if (value instanceof Map || value instanceof List) {
+ // Already-parsed JSON value (e.g. a Map cached on the GenericRow
before it was serialized for the forward
+ // index): flatten it directly, avoiding the serialize-then-reparse
round-trip.
+ addParsed(value);
} else {
+ // String (the common case) or, for any other unexpected type, fail
fast with a ClassCastException as before.
add((String) value);
}
Review Comment:
MutableJsonIndex.add(Object, ...) now dispatches Map/List values to
addParsed(), but it still treats JsonNode as an unexpected type and will
ClassCastException when cast to String. Since JsonUtils.flattenParsed()
supports JsonNode, it would be safer and more consistent to route JsonNode
through addParsed() as well (default addParsed will serialize+reparse if an
implementation doesn't optimize it).
--
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]
