xiangfu0 commented on code in PR #19014:
URL: https://github.com/apache/pinot/pull/19014#discussion_r3611310587
##########
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:
Fixed in a1b5ce18b1 — now implements
`Comparable<SerializedFrequentStringsSketch>` with `Integer.compare`, matching
the pattern of every other `Serialized*` custom object.
##########
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:
Fixed in a1b5ce18b1 — now implements
`Comparable<SerializedFrequentLongsSketch>` with `Integer.compare`, matching
the pattern of every other `Serialized*` custom object.
##########
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:
Confirmed — this is a pre-existing bug (also flagged during our review; the
filter/row index mix-up predates this PR, which only renamed a type inside the
block). It is being fixed with a regression test in a separate PR to keep this
one a mechanical platform upgrade; see the Follow-ups section in the
description.
--
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]
