You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2025/11/07 07:04:34 UTC
Re: [PR] feat(common): Enhance ArrayAgg to support aggregation on multi-value columns [pinot]
Copilot commented on code in PR #17153:
URL: https://github.com/apache/pinot/pull/17153#discussion_r2501917430
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ArrayAggStringFunction.java:
##########
@@ -38,11 +38,24 @@ public ArrayAggStringFunction(ExpressionContext expression, boolean nullHandling
public void aggregate(int length, AggregationResultHolder aggregationResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
BlockValSet blockValSet = blockValSetMap.get(_expression);
- String[] value = blockValSet.getStringValuesSV();
ObjectArrayList<String> valueArray =
aggregationResultHolder.getResult() != null ? aggregationResultHolder.getResult()
: new ObjectArrayList<>(length);
- forEachNotNull(length, blockValSet, (from, to) -> valueArray.addAll(Arrays.asList(value).subList(from, to)));
+ if (blockValSet.isSingleValue()) {
+ String[] values = blockValSet.getStringValuesSV();
+ forEachNotNull(length, blockValSet,
+ (from, to) -> valueArray.addAll(Arrays.asList(values).subList(from, to)));
Review Comment:
For single-value processing, consider using a more efficient approach than `Arrays.asList().subList()` which creates intermediate list wrappers. Use a loop to add elements directly: `for (int i = from; i < to; i++) { valueArray.add(values[i]); }`. This pattern is used consistently in the MV branch and other numeric function implementations.
```suggestion
(from, to) -> {
for (int i = from; i < to; i++) {
valueArray.add(values[i]);
}
});
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ArrayAggDistinctStringFunction.java:
##########
@@ -39,11 +39,24 @@ public ArrayAggDistinctStringFunction(ExpressionContext expression, boolean null
public void aggregate(int length, AggregationResultHolder aggregationResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
BlockValSet blockValSet = blockValSetMap.get(_expression);
- String[] value = blockValSet.getStringValuesSV();
ObjectOpenHashSet<String> valueSet =
aggregationResultHolder.getResult() != null ? aggregationResultHolder.getResult()
: new ObjectOpenHashSet<>(length);
- forEachNotNull(length, blockValSet, (from, to) -> valueSet.addAll(Arrays.asList(value).subList(from, to)));
+ if (blockValSet.isSingleValue()) {
+ String[] values = blockValSet.getStringValuesSV();
+ forEachNotNull(length, blockValSet,
+ (from, to) -> valueSet.addAll(Arrays.asList(values).subList(from, to)));
Review Comment:
Similar to the non-distinct variant, avoid creating intermediate list objects with `Arrays.asList().subList()`. Use a direct loop: `for (int i = from; i < to; i++) { valueSet.add(values[i]); }`. This maintains consistency with the numeric implementations and the MV processing branch.
```suggestion
(from, to) -> {
for (int i = from; i < to; i++) {
valueSet.add(values[i]);
}
});
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org