Copilot commented on code in PR #19169:
URL: https://github.com/apache/pinot/pull/19169#discussion_r3726687817
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/MapUtils.java:
##########
@@ -190,6 +194,120 @@ private static byte[] readLengthPrefixed(ByteBuffer
byteBuffer) {
return bytes;
}
+ private static void checkLength(ByteBuffer byteBuffer, int length) {
+ if (length < 0 || length > byteBuffer.remaining()) {
+ throw new BufferUnderflowException();
+ }
+ }
+
+ /// Renders a serialized MAP frame as a JSON object without materializing
the map.
+ ///
+ /// The frame already stores each value as the JSON bytes that
[#serializeMap] produced, so the values are copied
+ /// through verbatim and only the keys are quoted. That skips the
parse-into-`HashMap`-then-serialize-again round
+ /// trip [#toString(Map)] performs, and it skips Jackson entirely.
+ ///
Review Comment:
The Javadoc says this path "skips Jackson entirely", but the implementation
uses Jackson's `JsonStringEncoder` for escaping. Please adjust the wording to
avoid implying a complete Jackson removal (it’s avoiding databind/parsing and
map materialization, not all Jackson code).
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/MapUtils.java:
##########
@@ -190,6 +194,120 @@ private static byte[] readLengthPrefixed(ByteBuffer
byteBuffer) {
return bytes;
}
+ private static void checkLength(ByteBuffer byteBuffer, int length) {
+ if (length < 0 || length > byteBuffer.remaining()) {
+ throw new BufferUnderflowException();
+ }
+ }
+
+ /// Renders a serialized MAP frame as a JSON object without materializing
the map.
+ ///
+ /// The frame already stores each value as the JSON bytes that
[#serializeMap] produced, so the values are copied
+ /// through verbatim and only the keys are quoted. That skips the
parse-into-`HashMap`-then-serialize-again round
+ /// trip [#toString(Map)] performs, and it skips Jackson entirely.
+ ///
+ /// Entries are emitted in frame order. Both forward-index write paths
(`ForwardIndexCreator#putValue` at segment
+ /// build and `MutableSegmentImpl` while consuming) frame maps through the
key-sorting [#serializeMap(Map)], so for
+ /// those frames this is byte-identical to
`toString(deserializeMap(frame))`. A frame written through
+ /// [#serializeMap(Map, boolean)] with `sortByKey = false` renders in its
own insertion order instead.
+ public static String frameToJsonString(byte[] bytes) {
+ return frameToJsonString(ByteBuffer.wrap(bytes));
+ }
+
+ /// Variant of [#frameToJsonString(byte\[\])] reading from the buffer's
current position.
+ public static String frameToJsonString(ByteBuffer byteBuffer) {
+ byteBuffer.order(ByteOrder.BIG_ENDIAN);
+ int size = byteBuffer.getInt();
+ if (size == 0) {
+ return "{}";
+ }
Review Comment:
`frameToJsonString(ByteBuffer)` treats a negative frame entry count as an
empty map (the loop is skipped), returning "{}" and potentially masking
corrupted data. The legacy path (`deserializeMap`) would throw when given a
negative size, so this should reject negative `size` as well.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/MapUtils.java:
##########
@@ -190,6 +194,120 @@ private static byte[] readLengthPrefixed(ByteBuffer
byteBuffer) {
return bytes;
}
+ private static void checkLength(ByteBuffer byteBuffer, int length) {
+ if (length < 0 || length > byteBuffer.remaining()) {
+ throw new BufferUnderflowException();
+ }
+ }
+
+ /// Renders a serialized MAP frame as a JSON object without materializing
the map.
+ ///
+ /// The frame already stores each value as the JSON bytes that
[#serializeMap] produced, so the values are copied
+ /// through verbatim and only the keys are quoted. That skips the
parse-into-`HashMap`-then-serialize-again round
+ /// trip [#toString(Map)] performs, and it skips Jackson entirely.
+ ///
+ /// Entries are emitted in frame order. Both forward-index write paths
(`ForwardIndexCreator#putValue` at segment
+ /// build and `MutableSegmentImpl` while consuming) frame maps through the
key-sorting [#serializeMap(Map)], so for
+ /// those frames this is byte-identical to
`toString(deserializeMap(frame))`. A frame written through
+ /// [#serializeMap(Map, boolean)] with `sortByKey = false` renders in its
own insertion order instead.
+ public static String frameToJsonString(byte[] bytes) {
+ return frameToJsonString(ByteBuffer.wrap(bytes));
+ }
+
+ /// Variant of [#frameToJsonString(byte\[\])] reading from the buffer's
current position.
+ public static String frameToJsonString(ByteBuffer byteBuffer) {
+ byteBuffer.order(ByteOrder.BIG_ENDIAN);
+ int size = byteBuffer.getInt();
+ if (size == 0) {
+ return "{}";
+ }
+ // Quoting a key adds 2 bytes and the separators add 2, while the two
length prefixes it replaces free up 8, so
+ // an unescaped rendering never exceeds the remaining frame bytes.
Escaping is the only path that can grow.
+ JsonBuilder builder = new JsonBuilder(byteBuffer.remaining() + 2);
+ builder.append((byte) '{');
+ for (int i = 0; i < size; i++) {
+ if (i > 0) {
+ builder.append((byte) ',');
+ }
+ int keyLength = byteBuffer.getInt();
+ checkLength(byteBuffer, keyLength);
+ builder.appendQuotedKey(byteBuffer, keyLength);
+ byteBuffer.position(byteBuffer.position() + keyLength);
+ builder.append((byte) ':');
+ int valueLength = byteBuffer.getInt();
+ checkLength(byteBuffer, valueLength);
+ builder.appendRaw(byteBuffer, valueLength);
+ byteBuffer.position(byteBuffer.position() + valueLength);
+ }
+ builder.append((byte) '}');
+ return builder.toUtf8String();
+ }
+
+ /// Growable byte sink for [#frameToJsonString]. Assembling UTF-8 bytes and
decoding once at the end avoids
+ /// decoding every key individually, which is what makes the common
all-ASCII frame allocation-light.
+ private static final class JsonBuilder {
+ private byte[] _bytes;
+ private int _length;
+
+ private JsonBuilder(int capacity) {
+ _bytes = new byte[capacity];
+ }
+
+ private void ensure(int extra) {
+ if (_length + extra > _bytes.length) {
+ _bytes = Arrays.copyOf(_bytes, Math.max(_length + extra, _bytes.length
* 2));
+ }
+ }
+
+ private void append(byte b) {
+ ensure(1);
+ _bytes[_length++] = b;
+ }
+
+ /// Copies `length` bytes from the buffer's current position without
advancing it.
+ private void appendRaw(ByteBuffer byteBuffer, int length) {
+ ensure(length);
+ int offset = byteBuffer.position();
+ for (int i = 0; i < length; i++) {
+ _bytes[_length++] = byteBuffer.get(offset + i);
+ }
+ }
Review Comment:
`JsonBuilder.appendRaw` copies bytes one-by-one via `byteBuffer.get(offset +
i)`, which adds per-byte bounds checks and virtual calls in a hot loop. Since
correctness doesn’t depend on preserving the source buffer’s position, you can
use a `duplicate()` and a bulk `get(...)` into `_bytes` for significantly
cheaper copying.
--
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]
