Copilot commented on code in PR #19217:
URL: https://github.com/apache/pinot/pull/19217#discussion_r3761886806
##########
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:
`forEachNotNull` still invokes the callback with `(0, 0)` when a zero-length
block has no null bitmap. Creating the accumulator before verifying the range
is non-empty therefore marks an empty input as aggregated: with null handling
enabled, the raw function emits a serialized empty sketch and `SUMVALUES...`
emits `0` instead of `NULL`. Keep the holder untouched for an empty range.
This issue also appears on line 233 of the same file.
##########
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:
`deserializeSketch` is now inside the group-key loop, so a row assigned to N
group keys heapifies the same serialized sketch N times. This regresses the
previous once-per-row behavior on a query hot path; deserialize once before the
inner loop and reuse the sketch.
##########
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:
The new unit test directly instantiates these functions, so it does not
verify that this factory plumbing reaches server aggregation and broker
rendering. The existing `TupleSketchTest` integration data contains no null
BYTES values. Add end-to-end cases for both query engines with nullable sketch
rows, covering enabled-mode skipping/all-null answers and disabled-mode legacy
identities.
--
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]
