Jackie-Jiang commented on code in PR #19316:
URL: https://github.com/apache/pinot/pull/19316#discussion_r3824725895
##########
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:
Correcting my earlier reply on this thread: all seventeen are fixed here,
not deferred. I said they would go in a separate PR — that is no longer true,
and I did not want the thread left claiming otherwise.
The reasoning that changed it is the one you gave. Once the crash-inducing
subscript had to be pulled forward, the remaining fifteen were the same
one-token error in the same method, and shipping a method where two subscripts
are right and fifteen are still wrong is harder to review, not easier.
`.get(filterIndex + 1)` is untouched throughout, since that ordinal is
genuinely a filter index.
Tests are in `DistinctCountThetaSketchAggregationFunctionTest`: filtered
group-by-MV for the single-value, multi-value and string branches. Against the
pre-fix commit they fail with the specific wrong answers rather than by
throwing — the single-value case collapses all three rows onto row 0's group
(`expected 1, found 3`) and the multi-value cases send everything to group 0
(`expected 3, found 0`). The null-hole NPE case is in
`DistinctCountSketchNullHandlingTest`.
The PR title and body now call out that a filtered
`DISTINCTCOUNTTHETASKETCH` grouped by a multi-value column changes results,
independently of null handling. `backward-incompat` and `release-notes` were
already on the PR.
Happy to split it back out if you would still rather review the indexing fix
on its own.
--
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]
