gortiz commented on code in PR #12271:
URL: https://github.com/apache/pinot/pull/12271#discussion_r1528758023
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/PercentileEstAggregationFunction.java:
##########
@@ -133,28 +144,32 @@ public void aggregateGroupByMV(int length, int[][]
groupKeysArray, GroupByResult
BlockValSet blockValSet = blockValSetMap.get(_expression);
if (blockValSet.getValueType() != DataType.BYTES) {
long[] longValues = blockValSet.getLongValuesSV();
- for (int i = 0; i < length; i++) {
- long value = longValues[i];
- for (int groupKey : groupKeysArray[i]) {
- getDefaultQuantileDigest(groupByResultHolder, groupKey).add(value);
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ long value = longValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ getDefaultQuantileDigest(groupByResultHolder, groupKey).add(value);
+ }
}
- }
+ });
} else {
// Serialized QuantileDigest
byte[][] bytesValues = blockValSet.getBytesValuesSV();
- for (int i = 0; i < length; i++) {
- QuantileDigest value =
ObjectSerDeUtils.QUANTILE_DIGEST_SER_DE.deserialize(bytesValues[i]);
- for (int groupKey : groupKeysArray[i]) {
- QuantileDigest quantileDigest =
groupByResultHolder.getResult(groupKey);
- if (quantileDigest != null) {
- quantileDigest.merge(value);
- } else {
- // Create a new QuantileDigest for the group
- groupByResultHolder
- .setValueForKey(groupKey,
ObjectSerDeUtils.QUANTILE_DIGEST_SER_DE.deserialize(bytesValues[i]));
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ QuantileDigest value =
ObjectSerDeUtils.QUANTILE_DIGEST_SER_DE.deserialize(bytesValues[i]);
+ for (int groupKey : groupKeysArray[i]) {
+ QuantileDigest quantileDigest =
groupByResultHolder.getResult(groupKey);
+ if (quantileDigest != null) {
+ quantileDigest.merge(value);
+ } else {
+ // Create a new QuantileDigest for the group
+ groupByResultHolder.setValueForKey(groupKey,
+
ObjectSerDeUtils.QUANTILE_DIGEST_SER_DE.deserialize(bytesValues[i]));
Review Comment:
cc @Jackie-Jiang There is a very strange thing here: I've tried to change
this line to
```
groupByResultHolder.setValueForKey(groupKey, value);
```
but it randomly fails in `testInnerSegmentGroupByMV`. AFAIU
`ObjectSerDeUtils.QUANTILE_DIGEST_SER_DE.deserialize(bytesValues[i])` should be
pure and nobody should have changed `bytesValues[i]`. Do you see where is the
difference?
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
