Copilot commented on code in PR #18996:
URL: https://github.com/apache/pinot/pull/18996#discussion_r3591440895
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileTDigestAggregationFunction.java:
##########
@@ -215,15 +205,9 @@ public void aggregateGroupByMV(int length, int[][]
groupKeysArray, GroupByResult
byte[][] bytesValues = blockValSet.getBytesValuesSV();
forEachNotNull(length, blockValSet, (from, to) -> {
for (int i = from; i < to; i++) {
- TDigest value =
ObjectSerDeUtils.TDIGEST_SER_DE.deserialize(bytesValues[i]);
for (int groupKey : groupKeysArray[i]) {
- TDigest tDigest = groupByResultHolder.getResult(groupKey);
- if (tDigest != null) {
- tDigest.add(value);
- } else {
- // Create a new TDigest for the group
- groupByResultHolder.setValueForKey(groupKey,
ObjectSerDeUtils.TDIGEST_SER_DE.deserialize(bytesValues[i]));
- }
+ getSerializedAccumulator(groupByResultHolder, groupKey,
bytesValues[i])
+ .addSerializedTDigest(bytesValues[i]);
}
Review Comment:
In the BYTES + group-by-MV path, the same serialized TDigest is parsed once
per group key (`addSerializedTDigest(bytesValues[i])` inside the inner `for
(int groupKey : groupKeysArray[i])`). This is a regression vs. the previous
implementation that deserialized once per row and re-used the decoded
representation across all group keys for that row. For multi-value group-bys
with many keys per row, this can reintroduce significant per-row overhead and
undermine the intended allocation/CPU savings of the serialized merge path.
Consider decoding the serialized digest once per row and reusing the decoded
centroids when merging into multiple groups (e.g., add an accumulator API that
accepts already-decoded centroid arrays / a shared decode buffer, or a helper
that decodes into temporary primitive arrays once and then merges them into
each group accumulator).
--
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]
