Copilot commented on code in PR #19014:
URL: https://github.com/apache/pinot/pull/19014#discussion_r3611299392
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/SerializedFrequentLongsSketch.java:
##########
@@ -19,18 +19,18 @@
package org.apache.pinot.segment.local.customobject;
import java.util.Base64;
-import org.apache.datasketches.frequencies.LongsSketch;
+import org.apache.datasketches.frequencies.FrequentLongsSketch;
-public class SerializedFrequentLongsSketch implements Comparable<LongsSketch> {
- private final LongsSketch _sketch;
+public class SerializedFrequentLongsSketch implements
Comparable<FrequentLongsSketch> {
+ private final FrequentLongsSketch _sketch;
- public SerializedFrequentLongsSketch(LongsSketch sketch) {
+ public SerializedFrequentLongsSketch(FrequentLongsSketch sketch) {
_sketch = sketch;
}
@Override
- public int compareTo(LongsSketch other) {
+ public int compareTo(FrequentLongsSketch other) {
// There is no well-defined ordering for these sketches
// numActiveItems is just a placeholder, which can be changed later
return _sketch.getNumActiveItems() - other.getNumActiveItems();
Review Comment:
SerializedFrequentLongsSketch implements Comparable<FrequentLongsSketch>,
but the objects being compared at runtime are likely other
SerializedFrequentLongsSketch instances. This mismatch can cause
ClassCastException during result ordering. Implement Comparable against the
wrapper type instead, and use Integer.compare to avoid overflow.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/customobject/SerializedFrequentStringsSketch.java:
##########
@@ -20,17 +20,17 @@
import java.util.Base64;
import org.apache.datasketches.common.ArrayOfStringsSerDe;
-import org.apache.datasketches.frequencies.ItemsSketch;
+import org.apache.datasketches.frequencies.FrequentItemsSketch;
-public class SerializedFrequentStringsSketch implements
Comparable<ItemsSketch<String>> {
- private final ItemsSketch<String> _sketch;
+public class SerializedFrequentStringsSketch implements
Comparable<FrequentItemsSketch<String>> {
+ private final FrequentItemsSketch<String> _sketch;
- public SerializedFrequentStringsSketch(ItemsSketch<String> sketch) {
+ public SerializedFrequentStringsSketch(FrequentItemsSketch<String> sketch) {
_sketch = sketch;
}
@Override
- public int compareTo(ItemsSketch<String> other) {
+ public int compareTo(FrequentItemsSketch<String> other) {
// There is no well-defined ordering for these sketches
// numActiveItems is just a placeholder, which can be changed later
return _sketch.getNumActiveItems() - other.getNumActiveItems();
Review Comment:
SerializedFrequentStringsSketch implements
Comparable<FrequentItemsSketch<String>>, but the objects being compared at
runtime are likely other SerializedFrequentStringsSketch instances (e.g. when
sorting query results). This mismatch can cause ClassCastException. Implement
Comparable against the wrapper type instead, and use Integer.compare to avoid
overflow.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -799,7 +799,7 @@ public void aggregateGroupByMV(int length, int[][]
groupKeysArray, GroupByResult
for (int j = 0; j < length; j++) {
if (filterEvaluator.evaluate(singleValues, valueTypes,
valueArrays, j)) {
for (int groupKey : groupKeysArray[i]) {
- UpdateSketch updateSketch =
getUpdateSketches(groupByResultHolder, groupKey).get(i + 1);
+ UpdatableThetaSketch updateSketch =
getUpdateSketches(groupByResultHolder, groupKey).get(i + 1);
for (int value : intValues[i]) {
updateSketch.update(value);
Review Comment:
In aggregateGroupByMV (multi-value input path), the filter loop iterates
rows with index j but uses groupKeysArray[i] / intValues[i] (filter index)
instead of groupKeysArray[j] / intValues[j] (row index). This can union the
wrong values into the wrong groups. The same row-index vs filter-index mixup
appears in other data-type branches in this method as well.
--
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]
