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: [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]
