Copilot commented on code in PR #19171:
URL: https://github.com/apache/pinot/pull/19171#discussion_r3726686332
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/map/MapKeyIndexReader.java:
##########
@@ -56,29 +55,42 @@ public FieldSpec.DataType getStoredType() {
return _keyFieldSpec.getDataType().getStoredType();
}
+ // The numeric accessors below fast-path the type Jackson already produced
for this JSON shape - Integer for a
+ // small integer, Long for a large one, Double for a decimal - instead of
formatting it to a string and reparsing.
+ // Any other type still goes through the string round trip, so a value that
does not match the declared type
+ // fails exactly as it did before rather than being silently coerced.
+
@Override
public int getInt(int docId, ForwardIndexReaderContext context) {
- return Integer.parseInt(extractMapValue(docId, context,
_keyName).toString());
+ Object value = extractMapValue(docId, context, _keyName);
+ return value instanceof Integer ? (Integer) value :
Integer.parseInt(value.toString());
}
@Override
public long getLong(int docId, ForwardIndexReaderContext context) {
- return Long.parseLong(extractMapValue(docId, context,
_keyName).toString());
+ Object value = extractMapValue(docId, context, _keyName);
+ if (value instanceof Long) {
+ return (Long) value;
+ }
+ return value instanceof Integer ? (Integer) value :
Long.parseLong(value.toString());
}
@Override
public float getFloat(int docId, ForwardIndexReaderContext context) {
- return Float.parseFloat(extractMapValue(docId, context,
_keyName).toString());
+ Object value = extractMapValue(docId, context, _keyName);
+ return value instanceof Float ? (Float) value :
Float.parseFloat(value.toString());
}
Review Comment:
`getFloat()` fast-path checks `Float`, but Jackson numeric deserialization
for JSON typically yields `Double` (and sometimes `Integer`/`Long`) rather than
`Float`. As written this method will almost always fall back to
`Float.parseFloat(value.toString())`, missing the intended “avoid
toString+parse” optimization described in the comment above.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/MapUtils.java:
##########
@@ -183,13 +187,139 @@ public static Map<String, Object>
deserializeMap(ByteBuffer byteBuffer) {
return map;
}
+ /// Deserializes only the value for the requested key from a length-prefixed
MAP frame.
+ /// Non-matching keys and values are skipped without allocating byte arrays
or invoking Jackson.
+ ///
+ /// @param bytes Serialized MAP frame
+ /// @param key Key whose value should be deserialized
+ /// @return Deserialized value, or `null` if the key is missing, has a null
value, or cannot be deserialized
+ @Nullable
+ public static Object deserializeMapValue(byte[] bytes, String key) {
+ return deserializeMapValue(ByteBuffer.wrap(bytes), key);
+ }
+
+ /// Variant of [#deserializeMapValue(byte[], String)] that reads from the
supplied buffer without copying the
+ /// complete MAP frame.
+ ///
+ /// Consumes the buffer from its current position and forces
[ByteOrder#BIG_ENDIAN] on it — the write path frames
+ /// lengths through a big-endian [ByteBuffer], while an off-heap view
inherits the platform's native order.
+ @Nullable
+ public static Object deserializeMapValue(ByteBuffer byteBuffer, String key) {
+ byte[] valueBytes = findValueBytes(byteBuffer, key);
+ if (valueBytes == null) {
+ return null;
+ }
+ try {
+ return JsonUtils.bytesToObject(valueBytes, Object.class);
+ } catch (IOException e) {
+ LOGGER.error("Caught exception while deserializing value for key: {}",
key, e);
+ return null;
+ }
+ }
+
+ /// Reads the value for a key as a string, skipping Jackson when the stored
value is a plain JSON string.
+ ///
+ /// A `MAP` column with a `STRING` value type stores `"pinot-server"`, and
parsing that into a `String` only to
+ /// call `toString()` on it costs a parser instantiation per access. Values
that are not plain strings - numbers,
+ /// booleans, objects, arrays - still go through Jackson so the rendering
matches [#deserializeMapValue] exactly.
+ ///
+ /// @return The value as a string, or `null` if the key is missing, its
value is null, or it cannot be deserialized
+ @Nullable
+ public static String deserializeMapValueAsString(ByteBuffer byteBuffer,
String key) {
+ byte[] valueBytes = findValueBytes(byteBuffer, key);
+ if (valueBytes == null) {
+ return null;
+ }
+ String unquoted = unquotePlainJsonString(valueBytes);
+ if (unquoted != null) {
+ return unquoted;
+ }
+ try {
+ Object value = JsonUtils.bytesToObject(valueBytes, Object.class);
+ return value == null ? null : value.toString();
+ } catch (IOException e) {
+ LOGGER.error("Caught exception while deserializing value for key: {}",
key, e);
+ return null;
+ }
+ }
+
+ /// Decodes a JSON string literal that carries no escapes, otherwise returns
`null` so the caller falls back to
+ /// Jackson. A lone backslash anywhere means an escape sequence is present
and the literal is not its own value.
+ @Nullable
+ private static String unquotePlainJsonString(byte[] valueBytes) {
+ int length = valueBytes.length;
+ if (length < 2 || valueBytes[0] != '"' || valueBytes[length - 1] != '"') {
+ return null;
+ }
+ for (int i = 1; i < length - 1; i++) {
+ if (valueBytes[i] == '\\') {
+ return null;
+ }
+ }
+ return new String(valueBytes, 1, length - 2, StandardCharsets.UTF_8);
+ }
+
+ /// Scans a frame for `key` and returns its raw value bytes, or `null` when
the key is absent.
+ @Nullable
+ private static byte[] findValueBytes(ByteBuffer byteBuffer, String key) {
+ byteBuffer.order(ByteOrder.BIG_ENDIAN);
+ int size = byteBuffer.getInt();
+ if (size == 0) {
+ return null;
+ }
+ byte[] keyBytes = Utf8Utils.encode(key);
+ int keyBytesLength = keyBytes.length;
+ for (int i = 0; i < size; i++) {
+ int keyLength = byteBuffer.getInt();
+ // Bounds-check up front so the absolute gets below are provably in
range, and so a truncated frame still
+ // surfaces as BufferUnderflowException rather than
IndexOutOfBoundsException.
+ checkLength(byteBuffer, keyLength);
+ // Compare through absolute gets so a length mismatch or a differing
byte skips the rest of the key outright,
+ // rather than walking it one relative get at a time just to advance the
position.
+ boolean matches = keyLength == keyBytesLength;
+ if (matches) {
+ int keyOffset = byteBuffer.position();
+ for (int j = 0; j < keyLength; j++) {
+ if (byteBuffer.get(keyOffset + j) != keyBytes[j]) {
+ matches = false;
+ break;
+ }
+ }
+ }
+ byteBuffer.position(byteBuffer.position() + keyLength);
+
+ int valueLength = byteBuffer.getInt();
+ if (!matches) {
+ skip(byteBuffer, valueLength);
+ continue;
+ }
+ // Keys within a frame are unique - the write path iterates a Map - so
the first match is the only match and
+ // the remaining entries never need to be scanned.
+ byte[] valueBytes = new byte[valueLength];
+ byteBuffer.get(valueBytes);
+ return valueBytes;
Review Comment:
`findValueBytes` allocates `new byte[valueLength]` without validating
`valueLength` first. If the frame is truncated or corrupted, a negative/huge
`valueLength` can cause `NegativeArraySizeException`/OOM before the
`ByteBuffer.get(...)` underflow check triggers. Please reuse `checkLength` on
the matching path too (as the non-matching path already does).
--
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]
