Jackie-Jiang commented on code in PR #19316:
URL: https://github.com/apache/pinot/pull/19316#discussion_r3817853767
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java:
##########
@@ -462,18 +512,21 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
for (int i = 0; i < numFilters; i++) {
FilterEvaluator filterEvaluator = _filterEvaluators.get(i);
ThetaSketchAccumulator thetaSketchAccumulator =
thetaSketchAccumulators.get(i + 1);
- for (int j = 0; j < length; j++) {
- if (filterEvaluator.evaluate(singleValues, valueTypes, valueArrays,
j)) {
- thetaSketchAccumulator.apply(sketches[j]);
+ forEachNotNull(length, mainBlockValSet, (from, to) -> {
Review Comment:
Fixed. The default-sketch loop is inside `forEachNotNull` now, and
`deserializeSketches` takes the `BlockValSet` so a null row is never wrapped —
which also covers your second point about the empty-array default reaching
`ThetaSketch.wrap`. That one was a live crash, not just an accounting error: a
`BYTES` column whose null rows carry the empty default threw
`SketchesArgumentException: Possible Corruption: Given MemorySegment is empty`
before any filter could skip the row.
Covered by `testThetaDefaultSketchMergesOnlyNotNullRows` and
`testThetaNullRowWithEmptyDefaultIsNotDeserialized`.
On how it got missed: I had been enumerating `for (int i = ...)` loops, so
an enhanced-for over the deserialized array never matched the pattern I was
auditing with. Re-swept by what the code reads instead of by loop shape, which
is what turned up this one and the CPC one below.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountCPCSketchAggregationFunction.java:
##########
@@ -167,7 +168,8 @@ protected void aggregateSV(int length,
AggregationResultHolder aggregationResult
Dictionary dictionary = blockValSet.isDictionaryEncoded() ?
blockValSet.getDictionary() : null;
if (dictionary != null) {
int[] dictIds = blockValSet.getDictionaryIdsSV();
- getDictIdBitmap(aggregationResultHolder, dictionary).addN(dictIds, 0,
length);
+ forEachNotNull(length, blockValSet,
Review Comment:
Fixed — the branch is wrapped, and `deserializeSketches` now takes the
`BlockValSet` and skips null rows rather than relying on `bytes.length > 0`.
Your reasoning about the non-empty `defaultNullValue` is exactly right, and
it is what `testCpcSerializedRowsMergedOnlyWhenNotNull` pins: the payload
happening to be empty is incidental, so the row has to be excluded by the null
bitmap.
##########
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, and agreed it deserves its own fix — I have deliberately left all 17
spots alone here so this PR stays a null-handling change and does not quietly
alter query results for filtered `DISTINCTCOUNTTHETASKETCH` group-by-MV. Only
the loop variable rename is in this diff.
I will send the indexing fix as a separate PR under #19218 so it gets
reviewed on its own merits, since it needs `backward-incompat` /
`release-notes` labels that this one does not.
--
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]
