Jackie-Jiang commented on code in PR #20248:
URL: https://github.com/apache/pinot/pull/20248#discussion_r4569349833


##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/IntermediateResultByteBufferSerDeTest.java:
##########
@@ -487,12 +628,54 @@ public void testParentExprMinMax()
     
assertEquals(function.serializeIntermediateResult(deserialized).getBytes(), 
expectedBytes);
   }
 
+  @DataProvider
+  public static Object[][] offHeapSets() {
+    OffHeap32BitSet set32 = new OffHeap32BitSet(4);
+    OffHeap64BitSet set64 = new OffHeap64BitSet(4);
+    OffHeap128BitSet set128 = new OffHeap128BitSet(4);
+    // Includes zero, which the sets hold outside their hash tables
+    for (int value = 0; value < 3; value++) {
+      set32.add(value);
+      set64.add(value);
+      set128.add(value, -value);
+    }
+    return new Object[][]{
+        {set32, 0},
+        {set64, 1},
+        {set128, 2}
+    };
+  }
+
+  /// The `DISTINCTCOUNTOFFHEAP` function, checked on one serialization of 
each set, since serializing a set frees its
+  /// memory: its wire type value, the payload size, and a payload that 
deserializes to a set with the same final
+  /// result after its source buffer is overwritten.
+  @Test(dataProvider = "offHeapSets")
+  public void testDistinctCountOffHeap(BaseOffHeapSet set, int expectedType) {
+    DistinctCountOffHeapAggregationFunction function =
+        new DistinctCountOffHeapAggregationFunction(List.of(COLUMN), false);
+    int expectedFinalResult = set.size();
+    SerializedResult serialized = function.serializeIntermediateResult(set);
+    assertEquals(serialized.getType(), expectedType);
+    byte[] bytes = serialized.getBytes();
+    assertEquals(serialized.getSize(), bytes.length);

Review Comment:
   Good catch, thanks. `offHeapSets()` now gives each set its expected payload 
length (`Integer.BYTES` plus 4, 8 or 16 bytes per value), and the test asserts 
both `getSize()` and `bytes.length` against it. Without #20203 the 32-bit set 
serializes to 28 bytes instead of 16, so this fails now. I also updated the PR 
description to match.
   



-- 
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]

Reply via email to