yashmayya commented on code in PR #19316:
URL: https://github.com/apache/pinot/pull/19316#discussion_r3823444539
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -877,158 +987,193 @@ public void aggregateGroupByMV(int length, int[][]
groupKeysArray, GroupByResult
case INT:
int[][] intValues = (int[][]) valueArrays[0];
if (_includeDefaultSketch) {
- for (int i = 0; i < length; i++) {
- for (int groupKey : groupKeysArray[i]) {
- UpdatableThetaSketch defaultSketch =
getUpdateSketches(groupByResultHolder, groupKey).get(0);
- for (int value : intValues[i]) {
- defaultSketch.update(value);
+ forEachNotNull(length, mainBlockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (int groupKey : groupKeysArray[i]) {
+ UpdatableThetaSketch defaultSketch =
getUpdateSketches(groupByResultHolder, groupKey).get(0);
+ for (int value : intValues[i]) {
+ defaultSketch.update(value);
+ }
}
}
- }
+ });
}
for (int i = 0; i < numFilters; i++) {
FilterEvaluator filterEvaluator = _filterEvaluators.get(i);
- for (int j = 0; j < length; j++) {
- if (filterEvaluator.evaluate(singleValues, valueTypes,
valueArrays, j)) {
- for (int groupKey : groupKeysArray[i]) {
- UpdatableThetaSketch updateSketch =
getUpdateSketches(groupByResultHolder, groupKey).get(i + 1);
- for (int value : intValues[i]) {
- updateSketch.update(value);
+ int filterIndex = i;
+ forEachNotNull(length, mainBlockValSet, (from, to) -> {
+ for (int j = from; j < to; j++) {
+ if (filterEvaluator.evaluate(singleValues, valueTypes,
valueArrays, j)) {
+ for (int groupKey : groupKeysArray[filterIndex]) {
Review Comment:
Agreed on deferring, with one exception that the fix itself created.
`deserializeSketches` now leaves `null` in the array for a null row. Line
877 reads that array at the wrong index:
```java
for (int j = from; j < to; j++) { // j is the row, never null
if (filterEvaluator.evaluate(..., j)) {
for (int groupKey : groupKeysArray[filterIndex]) {
getUnions(groupByResultHolder, groupKey).get(filterIndex +
1).apply(sketches[filterIndex]);
```
`filterIndex` is a filter ordinal, so it sits outside the range the loop
walks. With one filter it reads `sketches[0]`. If row 0 is null, that entry is
a hole, and `CustomObjectAccumulator.apply` opens with
`Preconditions.checkNotNull`.
So `DISTINCTCOUNTTHETASKETCH(sketchCol, 'pred')` with `GROUP BY mvCol`
throws an NPE when the option is on and the first row of a block is null.
Before this revision the array was full, so the same line returned a wrong
answer instead of throwing. No test hits that combination: filter, plus
multi-value group by, plus a null first row.
This is the only one of the 17 that reads the deserialized array. The other
16 read raw value arrays, which have no holes, so they stay wrong-but-safe
until the follow-up.
Worth pulling this one line forward to `groupKeysArray[j]` and
`sketches[j]`. The rest can go with the separate PR.
--
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]
