Copilot commented on code in PR #19168:
URL: https://github.com/apache/pinot/pull/19168#discussion_r3726692954
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/MapUtils.java:
##########
@@ -183,13 +186,87 @@ 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) {
+ 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);
Review Comment:
`deserializeMapValue(ByteBuffer, ...)` allocates `new byte[valueLength]`
without validating `valueLength` against the remaining buffer. For
malformed/truncated frames this can throw `NegativeArraySizeException` / OOME
instead of the intended `BufferUnderflowException` behavior used by the skip
path.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/MapUtils.java:
##########
@@ -183,13 +186,87 @@ 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) {
Review Comment:
The Javadoc for `deserializeMapValue` says it returns null when the value
"cannot be deserialized", but malformed/truncated frames will currently throw
`BufferUnderflowException` (similar to `deserializeMap`). Consider documenting
that exception explicitly so callers don't treat it as a missing-key `null`.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/map/MapKeyIndexReaderTest.java:
##########
@@ -0,0 +1,102 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.index.map;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext;
+import org.apache.pinot.spi.data.DimensionFieldSpec;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers [MapKeyIndexReader] over both shapes of underlying reader: one that
implements the selective
+/// [ForwardIndexReader#getMapValue] override (as the mutable forward index
does), and one that only implements
+/// [ForwardIndexReader#getMap] and therefore falls through to the default.
Both must agree.
+public class MapKeyIndexReaderTest {
+ private static final Map<String, Object> MAP =
+ Map.of("k8s.workload.name", "pinot-server", "k8s.workload.replicas", 3);
+
+ @Test
+ public void testSelectiveReader() {
+ assertReaderBehavior(new SelectiveReader());
+ }
+
+ /// The immutable sparse-key path inherits the default `getMapValue`. It has
to keep working unchanged.
+ @Test
+ public void testReaderWithoutSelectiveOverride() {
+ assertReaderBehavior(new FullMapOnlyReader());
+ }
+
+ private static void assertReaderBehavior(ForwardIndexReader reader) {
Review Comment:
This test uses the raw `ForwardIndexReader` type, which loses compile-time
type safety and introduces avoidable unchecked warnings. It can be
parameterized with `ForwardIndexReaderContext` here.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/map/MapKeyIndexReaderTest.java:
##########
@@ -0,0 +1,102 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.index.map;
+
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext;
+import org.apache.pinot.spi.data.DimensionFieldSpec;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Covers [MapKeyIndexReader] over both shapes of underlying reader: one that
implements the selective
+/// [ForwardIndexReader#getMapValue] override (as the mutable forward index
does), and one that only implements
+/// [ForwardIndexReader#getMap] and therefore falls through to the default.
Both must agree.
+public class MapKeyIndexReaderTest {
+ private static final Map<String, Object> MAP =
+ Map.of("k8s.workload.name", "pinot-server", "k8s.workload.replicas", 3);
+
+ @Test
+ public void testSelectiveReader() {
+ assertReaderBehavior(new SelectiveReader());
+ }
+
+ /// The immutable sparse-key path inherits the default `getMapValue`. It has
to keep working unchanged.
+ @Test
+ public void testReaderWithoutSelectiveOverride() {
+ assertReaderBehavior(new FullMapOnlyReader());
+ }
+
+ private static void assertReaderBehavior(ForwardIndexReader reader) {
+ FieldSpec stringSpec = new DimensionFieldSpec("value", DataType.STRING,
true);
+ assertEquals(new MapKeyIndexReader(reader, "k8s.workload.name",
stringSpec).getString(0, null), "pinot-server");
+
+ // A key that is absent from the map resolves to the field spec's default
null value, not to null.
+ assertEquals(new MapKeyIndexReader(reader, "missing",
stringSpec).getString(0, null),
+ stringSpec.getDefaultNullValue());
+
+ FieldSpec intSpec = new DimensionFieldSpec("value", DataType.INT, true);
+ assertEquals(new MapKeyIndexReader(reader, "k8s.workload.replicas",
intSpec).getInt(0, null), 3);
+ }
+
+ /// Mirrors the mutable forward index: answers a single key without
materializing the map.
+ private static class SelectiveReader extends BaseReader {
+ @Override
+ @Nullable
+ public Object getMapValue(int docId, ForwardIndexReaderContext context,
String key) {
+ return MAP.get(key);
+ }
+ }
+
+ /// Mirrors a reader that only knows how to hand back the whole map.
+ private static class FullMapOnlyReader extends BaseReader {
+ }
+
+ @SuppressWarnings("rawtypes")
+ private abstract static class BaseReader implements ForwardIndexReader {
Review Comment:
`BaseReader` implements `ForwardIndexReader` as a raw type and suppresses
the warning. Using `ForwardIndexReader<ForwardIndexReaderContext>` avoids the
suppression and keeps overrides type-checked.
--
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]
