Jackie-Jiang commented on code in PR #19217:
URL: https://github.com/apache/pinot/pull/19217#discussion_r3762480823
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java:
##########
@@ -156,11 +157,14 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
if (storedType == FieldSpec.DataType.BYTES) {
byte[][] bytesValues = blockValSet.getBytesValuesSV();
try {
- TupleIntSketchAccumulator tupleIntSketchAccumulator =
getAccumulator(aggregationResultHolder);
- TupleSketch<IntegerSummary>[] sketches =
deserializeSketches(bytesValues, length);
- for (TupleSketch<IntegerSummary> sketch : sketches) {
- tupleIntSketchAccumulator.apply(sketch);
- }
+ // the accumulator is created inside the range, so an all-null block
leaves the holder untouched and
+ // extractFinalResult sees the null that means nothing was aggregated
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ TupleIntSketchAccumulator tupleIntSketchAccumulator =
getAccumulator(aggregationResultHolder);
+ for (int i = from; i < to; i++) {
+ tupleIntSketchAccumulator.apply(deserializeSketch(bytesValues[i]));
+ }
Review Comment:
Correct, and it was a real hole. `forEachNotNull` calls the consumer with
`(0, 0)` for a zero-length block, so creating the accumulator inside the range
was not enough — the range itself can be empty, and the holder was marked as
aggregated for an input that aggregated nothing.
The range now returns early when `to == from`, before the accumulator exists.
`testZeroLengthBlockLeavesTheHolderUntouched` pins it, and I mutation-tested
the guard: removing it fails that test. Nothing else covered the case, so thank
you — this would have shipped.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/IntegerTupleSketchAggregationFunction.java:
##########
@@ -208,12 +211,13 @@ public void aggregateGroupByMV(int length, int[][]
groupKeysArray, GroupByResult
if (singleValue && storedType == FieldSpec.DataType.BYTES) {
byte[][] bytesValues =
blockValSetMap.get(_expression).getBytesValuesSV();
try {
- TupleSketch<IntegerSummary>[] sketches =
deserializeSketches(bytesValues, length);
- for (int i = 0; i < length; i++) {
- for (int groupKey : groupKeysArray[i]) {
- getAccumulator(groupByResultHolder, groupKey).apply(sketches[i]);
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (int groupKey : groupKeysArray[i]) {
+ getAccumulator(groupByResultHolder,
groupKey).apply(deserializeSketch(bytesValues[i]));
+ }
}
Review Comment:
Correct, and a regression I introduced. The previous code deserialized each
row once into an array before the loop; moving deserialization into the range
put it inside the group-key loop, so a row belonging to N group keys was
heapified N times.
Now deserialized once per row, above the inner loop, and reused for every
group key that row belongs to. The single-value group-by path was already once
per row and is unchanged.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -481,13 +481,16 @@ public static AggregationFunction
getAggregationFunction(FunctionContext functio
return new FourthMomentAggregationFunction(arguments,
FourthMomentAggregationFunction.Type.MOMENT);
case DISTINCTCOUNTTUPLESKETCH:
// mode actually doesn't matter here because we only care about
keys, not values
- return new
DistinctCountIntegerTupleSketchAggregationFunction(arguments,
IntegerSummary.Mode.Sum);
+ return new
DistinctCountIntegerTupleSketchAggregationFunction(arguments,
IntegerSummary.Mode.Sum,
Review Comment:
Agreed, and it is the honest gap in this PR. The coverage here is direct
construction, so it does not prove the factory plumbing reaches server
aggregation or that the rendered value survives to the broker.
I have not added end-to-end cases, because the existing tuple sketch
fixtures carry no null `BYTES` rows and building that fixture is a larger piece
than this change. Tracking it on #19218 rather than claiming coverage this PR
does not have — the description now says the unit test is the only coverage and
why the contract test cannot reach this family.
--
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]
