xiangfu0 commented on code in PR #19168:
URL: https://github.com/apache/pinot/pull/19168#discussion_r3771984942
##########
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:
Fixed in `0b0c2cd266`. The selective decoder now validates `valueLength`
with `checkLength(...)` before skipping or allocating, so negative and
oversized lengths consistently throw `BufferUnderflowException`.
`testDeserializeMapEntryValueRejectsInvalidValueLength` covers matching and
missing keys plus negative and truncated lengths.
##########
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:
Fixed in `0b0c2cd266`. `assertReaderBehavior` now accepts
`ForwardIndexReader<ForwardIndexReaderContext>`, preserving compile-time type
safety.
##########
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:
Fixed in `0b0c2cd266`. `BaseReader` now implements
`ForwardIndexReader<ForwardIndexReaderContext>`, and the raw-type suppression
was removed.
##########
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:
Fixed in `0b0c2cd266`. The selective deserializer Javadocs now explicitly
declare `BufferUnderflowException` for malformed or truncated MAP frames,
separately from the documented `null` result for an unparseable JSON value.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/io/writer/impl/MutableOffHeapByteArrayStore.java:
##########
@@ -233,6 +245,19 @@ public int getValueSize(int index) {
throw new RuntimeException("dictionary ID '" + index + "' too low");
}
+ /// Returns a view of the value at the given index without copying it.
+ /// The returned buffer must not be used after this store is closed.
+ public ByteBuffer getByteBuffer(int index) {
Review Comment:
Done in `0b0c2cd266`. The public `getByteBuffer(int)` method now appears
immediately before `getValueSize(int)`.
--
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]
