Copilot commented on code in PR #16843:
URL: https://github.com/apache/pinot/pull/16843#discussion_r2357287521
##########
pinot-core/src/test/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowSerDeTest.java:
##########
@@ -136,4 +146,87 @@ public void testCompare() {
assertEquals(deserializer.compare(0L, numBytes, i), 0);
}
}
+
+ @Test
+ public void testMapSerDe() {
+ List<FieldSpec> mapFieldSpecs = Arrays.asList(new
ComplexFieldSpec("mapSV", DataType.MAP, true, new HashMap<>()));
+
+ GenericRow mapRow = new GenericRow();
+ Map<String, Object> testMap = new HashMap<>();
+ testMap.put("stringKey", "stringValue");
+ testMap.put("intKey", 123);
+ testMap.put("doubleKey", 45.67);
+ mapRow.putValue("mapSV", testMap);
+
+ GenericRowSerializer serializer = new GenericRowSerializer(mapFieldSpecs,
false);
+ byte[] bytes = serializer.serialize(mapRow);
+ PinotDataBuffer dataBuffer = PinotDataBuffer.allocateDirect(bytes.length,
PinotDataBuffer.NATIVE_ORDER, null);
+ dataBuffer.readFrom(0L, bytes);
+ GenericRowDeserializer deserializer = new
GenericRowDeserializer(dataBuffer, mapFieldSpecs, false);
+ GenericRow buffer = new GenericRow();
+ deserializer.deserialize(0L, buffer);
+
+ @SuppressWarnings("unchecked")
+ Map<String, Object> deserializedMap = (Map<String, Object>)
buffer.getValue("mapSV");
+ assertEquals(deserializedMap, testMap);
+ }
+
+ @Test
+ public void testMapCompare() {
+ // Test MAP comparison for both different and equal maps
+ List<FieldSpec> mapFieldSpecs = Arrays.asList(new
ComplexFieldSpec("mapSV", DataType.MAP, true, new HashMap<>()));
+
+ GenericRow mapRow1 = new GenericRow();
+ Map<String, Object> testMap1 = new HashMap<>();
+ testMap1.put("key1", "value1");
+ testMap1.put("key2", 123);
+ mapRow1.putValue("mapSV", testMap1);
+
+ GenericRow mapRow2 = new GenericRow();
+ Map<String, Object> testMap2 = new HashMap<>();
+ testMap2.put("key1", "value2");
+ testMap2.put("key2", 123);
+ mapRow2.putValue("mapSV", testMap2);
+
+ GenericRowSerializer serializer = new GenericRowSerializer(mapFieldSpecs,
false);
+ byte[] bytes1 = serializer.serialize(mapRow1);
+ byte[] bytes2 = serializer.serialize(mapRow2);
+
+ long numBytes = Math.max(bytes1.length, bytes2.length);
+ PinotDataBuffer dataBuffer = PinotDataBuffer.allocateDirect(numBytes * 2,
PinotDataBuffer.NATIVE_ORDER, null);
+ dataBuffer.readFrom(0L, bytes1);
+ dataBuffer.readFrom(numBytes, bytes2);
+
+ GenericRowDeserializer deserializer = new
GenericRowDeserializer(dataBuffer, mapFieldSpecs, true);
+
+ // This should return non-zero as values are different
+ int result = deserializer.compare(0L, numBytes, 1);
Review Comment:
The third parameter should be the number of fields to compare, not a
hardcoded value. Since there's only one MAP field in the test, this should be
`mapFieldSpecs.size()` or explicitly `1` with a comment explaining why.
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowDeserializer.java:
##########
@@ -266,6 +278,23 @@ public int compare(long offset1, long offset2, int
numFieldsToCompare) {
offset2 += numBytes2;
break;
}
+ case MAP: {
+ int numBytes1 = _dataBuffer.getInt(offset1);
+ offset1 += Integer.BYTES;
+ byte[] mapBytes1 = new byte[numBytes1];
+ _dataBuffer.copyTo(offset1, mapBytes1);
+ int numBytes2 = _dataBuffer.getInt(offset2);
+ offset2 += Integer.BYTES;
+ byte[] mapBytes2 = new byte[numBytes2];
+ _dataBuffer.copyTo(offset2, mapBytes2);
+ int result = ByteArray.compare(mapBytes1, mapBytes2);
Review Comment:
Using byte-level comparison for MAPs may not provide meaningful ordering
semantics and could be inefficient for large maps. Consider if map comparison
should deserialize and compare the actual map contents, or document that this
comparison is only for deterministic ordering, not semantic equality.
##########
pinot-core/src/test/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowSerDeTest.java:
##########
@@ -136,4 +146,87 @@ public void testCompare() {
assertEquals(deserializer.compare(0L, numBytes, i), 0);
}
}
+
+ @Test
+ public void testMapSerDe() {
+ List<FieldSpec> mapFieldSpecs = Arrays.asList(new
ComplexFieldSpec("mapSV", DataType.MAP, true, new HashMap<>()));
+
+ GenericRow mapRow = new GenericRow();
+ Map<String, Object> testMap = new HashMap<>();
+ testMap.put("stringKey", "stringValue");
+ testMap.put("intKey", 123);
+ testMap.put("doubleKey", 45.67);
+ mapRow.putValue("mapSV", testMap);
+
+ GenericRowSerializer serializer = new GenericRowSerializer(mapFieldSpecs,
false);
+ byte[] bytes = serializer.serialize(mapRow);
+ PinotDataBuffer dataBuffer = PinotDataBuffer.allocateDirect(bytes.length,
PinotDataBuffer.NATIVE_ORDER, null);
+ dataBuffer.readFrom(0L, bytes);
+ GenericRowDeserializer deserializer = new
GenericRowDeserializer(dataBuffer, mapFieldSpecs, false);
+ GenericRow buffer = new GenericRow();
+ deserializer.deserialize(0L, buffer);
+
+ @SuppressWarnings("unchecked")
+ Map<String, Object> deserializedMap = (Map<String, Object>)
buffer.getValue("mapSV");
+ assertEquals(deserializedMap, testMap);
+ }
+
+ @Test
+ public void testMapCompare() {
+ // Test MAP comparison for both different and equal maps
+ List<FieldSpec> mapFieldSpecs = Arrays.asList(new
ComplexFieldSpec("mapSV", DataType.MAP, true, new HashMap<>()));
+
+ GenericRow mapRow1 = new GenericRow();
+ Map<String, Object> testMap1 = new HashMap<>();
+ testMap1.put("key1", "value1");
+ testMap1.put("key2", 123);
+ mapRow1.putValue("mapSV", testMap1);
+
+ GenericRow mapRow2 = new GenericRow();
+ Map<String, Object> testMap2 = new HashMap<>();
+ testMap2.put("key1", "value2");
+ testMap2.put("key2", 123);
+ mapRow2.putValue("mapSV", testMap2);
+
+ GenericRowSerializer serializer = new GenericRowSerializer(mapFieldSpecs,
false);
+ byte[] bytes1 = serializer.serialize(mapRow1);
+ byte[] bytes2 = serializer.serialize(mapRow2);
+
+ long numBytes = Math.max(bytes1.length, bytes2.length);
+ PinotDataBuffer dataBuffer = PinotDataBuffer.allocateDirect(numBytes * 2,
PinotDataBuffer.NATIVE_ORDER, null);
+ dataBuffer.readFrom(0L, bytes1);
+ dataBuffer.readFrom(numBytes, bytes2);
+
+ GenericRowDeserializer deserializer = new
GenericRowDeserializer(dataBuffer, mapFieldSpecs, true);
+
+ // This should return non-zero as values are different
+ int result = deserializer.compare(0L, numBytes, 1);
+ assertTrue(result != 0, "MAP comparison should return non-zero for
different maps");
+
+ // Test 2: Equal maps should return 0
+ GenericRow mapRow3 = new GenericRow();
+ Map<String, Object> testMap3 = new HashMap<>();
+ testMap3.put("key1", "value1");
+ testMap3.put("key2", 123);
+ mapRow3.putValue("mapSV", testMap3);
+
+ GenericRow mapRow4 = new GenericRow();
+ Map<String, Object> testMap4 = new HashMap<>();
+ testMap4.put("key1", "value1"); // Same value
+ testMap4.put("key2", 123); // Same value
+ mapRow4.putValue("mapSV", testMap4);
+
+ byte[] bytes3 = serializer.serialize(mapRow3);
+ byte[] bytes4 = serializer.serialize(mapRow4);
+
+ long numBytes2 = Math.max(bytes3.length, bytes4.length);
+ PinotDataBuffer dataBuffer2 = PinotDataBuffer.allocateDirect(numBytes2 *
2, PinotDataBuffer.NATIVE_ORDER, null);
+ dataBuffer2.readFrom(0L, bytes3);
+ dataBuffer2.readFrom(numBytes2, bytes4);
+
+ GenericRowDeserializer deserializer2 = new
GenericRowDeserializer(dataBuffer2, mapFieldSpecs, true);
+
+ int result2 = deserializer2.compare(0L, numBytes2, 1);
Review Comment:
Same issue as above - the third parameter should be `mapFieldSpecs.size()`
instead of hardcoded `1` for consistency and clarity.
```suggestion
int result2 = deserializer2.compare(0L, numBytes2, mapFieldSpecs.size());
```
--
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]
