Copilot commented on code in PR #17155:
URL: https://github.com/apache/pinot/pull/17155#discussion_r2496671318
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionFactory.java:
##########
@@ -285,6 +287,25 @@ public static AggregationFunction
getAggregationFunction(FunctionContext functio
}
return new ListAggFunction(arguments.get(0), separator,
nullHandlingEnabled);
}
+ case LISTAGGMV: {
+ Preconditions.checkArgument(numArguments == 2 || numArguments == 3,
+ "LISTAGG_MV expects 2 arguments, got: %s. The function can be
used as "
+ + "listAggMv([distinct] expression, 'separator')",
numArguments);
+ ExpressionContext separatorExpression = arguments.get(1);
+ Preconditions.checkArgument(separatorExpression.getType() ==
ExpressionContext.Type.LITERAL,
+ "LISTAGG_MV expects the 2nd argument to be literal, got: %s.
The function can be used as "
+ + "listAggMv([distinct] expression, 'separator')",
separatorExpression.getType());
Review Comment:
The error messages state "LISTAGG_MV expects 2 arguments" but the function
actually accepts 2 or 3 arguments (the third being the optional `distinct`
parameter). The message should clarify that it expects "2 or 3 arguments" to
avoid confusion when users provide the valid 3-argument form.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggMvFunction.java:
##########
@@ -0,0 +1,185 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.aggregation.function.array;
+
+import it.unimi.dsi.fastutil.objects.ObjectArrayList;
+import it.unimi.dsi.fastutil.objects.ObjectCollection;
+import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.CustomObject;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.ObjectSerDeUtils;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
+import
org.apache.pinot.core.query.aggregation.function.NullableSingleInputAggregationFunction;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import
org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+
+
+public class ListAggMvFunction extends
NullableSingleInputAggregationFunction<ObjectCollection<String>, String> {
+
+ private final String _separator;
+
+ public ListAggMvFunction(ExpressionContext expression, String separator,
boolean nullHandlingEnabled) {
+ super(expression, nullHandlingEnabled);
+ _separator = separator;
+ }
+
+ @Override
+ public AggregationFunctionType getType() {
+ return AggregationFunctionType.LISTAGGMV;
+ }
+
+ @Override
+ public AggregationResultHolder createAggregationResultHolder() {
+ return new ObjectAggregationResultHolder();
+ }
+
+ @Override
+ public GroupByResultHolder createGroupByResultHolder(int initialCapacity,
int maxCapacity) {
+ return new ObjectGroupByResultHolder(initialCapacity, maxCapacity);
+ }
+
+ @Override
+ public void aggregate(int length, AggregationResultHolder
aggregationResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ ObjectCollection<String> valuesOut =
getObjectCollection(aggregationResultHolder);
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+ String[][] valuesArray = blockValSet.getStringValuesMV();
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (String v : valuesArray[i]) {
+ valuesOut.add(v);
+ }
+ }
+ });
+ }
+
+ protected ObjectCollection<String>
getObjectCollection(AggregationResultHolder aggregationResultHolder) {
+ ObjectArrayList<String> valueList = aggregationResultHolder.getResult();
+ if (valueList == null) {
+ valueList = new ObjectArrayList<>();
+ aggregationResultHolder.setValue(valueList);
+ }
+ return valueList;
+ }
+
+ protected ObjectCollection<String> getObjectCollection(GroupByResultHolder
groupByResultHolder, int groupKey) {
+ ObjectArrayList<String> valueList =
groupByResultHolder.getResult(groupKey);
+ if (valueList == null) {
+ valueList = new ObjectArrayList<>();
+ groupByResultHolder.setValueForKey(groupKey, valueList);
+ }
+ return valueList;
+ }
+
+ @Override
+ public void aggregateGroupBySV(int length, int[] groupKeyArray,
GroupByResultHolder groupByResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+ String[][] valuesArray = blockValSet.getStringValuesMV();
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ ObjectCollection<String> groupValues =
getObjectCollection(groupByResultHolder, groupKeyArray[i]);
+ for (String v : valuesArray[i]) {
+ groupValues.add(v);
+ }
+ }
+ });
+ }
+
+ @Override
+ public void aggregateGroupByMV(int length, int[][] groupKeysArray,
GroupByResultHolder groupByResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+ String[][] valuesArray = blockValSet.getStringValuesMV();
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ int[] groupKeys = groupKeysArray[i];
+ for (int groupKey : groupKeys) {
+ ObjectCollection<String> groupValues =
getObjectCollection(groupByResultHolder, groupKey);
+ for (String v : valuesArray[i]) {
+ groupValues.add(v);
+ }
+ }
+ }
+ });
+ }
+
+ @Override
+ public ObjectCollection<String>
extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
+ return aggregationResultHolder.getResult();
+ }
+
+ @Override
+ public ObjectCollection<String> extractGroupByResult(GroupByResultHolder
groupByResultHolder, int groupKey) {
+ return groupByResultHolder.getResult(groupKey);
+ }
+
+ @Override
+ public ObjectCollection<String> merge(ObjectCollection<String>
intermediateResult1,
+ ObjectCollection<String> intermediateResult2) {
+ if (intermediateResult1 == null) {
+ return intermediateResult2;
+ }
+ if (intermediateResult2 == null) {
+ return intermediateResult1;
+ }
+ intermediateResult1.addAll(intermediateResult2);
+ return intermediateResult1;
+ }
+
+ @Override
+ public ColumnDataType getIntermediateResultColumnType() {
+ return ColumnDataType.OBJECT;
+ }
+
+ @Override
+ public SerializedIntermediateResult
serializeIntermediateResult(ObjectCollection<String> strings) {
+ ObjectArrayList<String> list = new ObjectArrayList<>();
+ for (String s : strings) {
+ list.add(s);
+ }
Review Comment:
Instead of iterating to copy elements into a new `ObjectArrayList`, use the
constructor that accepts a collection: `new ObjectArrayList<>(strings)`. This
is more efficient and eliminates unnecessary iteration.
```suggestion
ObjectArrayList<String> list = new ObjectArrayList<>(strings);
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/array/ListAggMvFunction.java:
##########
@@ -0,0 +1,185 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.aggregation.function.array;
+
+import it.unimi.dsi.fastutil.objects.ObjectArrayList;
+import it.unimi.dsi.fastutil.objects.ObjectCollection;
+import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.common.CustomObject;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.ObjectSerDeUtils;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
+import
org.apache.pinot.core.query.aggregation.function.NullableSingleInputAggregationFunction;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import
org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+
+
+public class ListAggMvFunction extends
NullableSingleInputAggregationFunction<ObjectCollection<String>, String> {
+
+ private final String _separator;
+
+ public ListAggMvFunction(ExpressionContext expression, String separator,
boolean nullHandlingEnabled) {
+ super(expression, nullHandlingEnabled);
+ _separator = separator;
+ }
+
+ @Override
+ public AggregationFunctionType getType() {
+ return AggregationFunctionType.LISTAGGMV;
+ }
+
+ @Override
+ public AggregationResultHolder createAggregationResultHolder() {
+ return new ObjectAggregationResultHolder();
+ }
+
+ @Override
+ public GroupByResultHolder createGroupByResultHolder(int initialCapacity,
int maxCapacity) {
+ return new ObjectGroupByResultHolder(initialCapacity, maxCapacity);
+ }
+
+ @Override
+ public void aggregate(int length, AggregationResultHolder
aggregationResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ ObjectCollection<String> valuesOut =
getObjectCollection(aggregationResultHolder);
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+ String[][] valuesArray = blockValSet.getStringValuesMV();
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ for (String v : valuesArray[i]) {
+ valuesOut.add(v);
+ }
+ }
+ });
+ }
+
+ protected ObjectCollection<String>
getObjectCollection(AggregationResultHolder aggregationResultHolder) {
+ ObjectArrayList<String> valueList = aggregationResultHolder.getResult();
+ if (valueList == null) {
+ valueList = new ObjectArrayList<>();
+ aggregationResultHolder.setValue(valueList);
+ }
+ return valueList;
+ }
+
+ protected ObjectCollection<String> getObjectCollection(GroupByResultHolder
groupByResultHolder, int groupKey) {
+ ObjectArrayList<String> valueList =
groupByResultHolder.getResult(groupKey);
+ if (valueList == null) {
+ valueList = new ObjectArrayList<>();
+ groupByResultHolder.setValueForKey(groupKey, valueList);
+ }
+ return valueList;
+ }
+
+ @Override
+ public void aggregateGroupBySV(int length, int[] groupKeyArray,
GroupByResultHolder groupByResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+ String[][] valuesArray = blockValSet.getStringValuesMV();
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ ObjectCollection<String> groupValues =
getObjectCollection(groupByResultHolder, groupKeyArray[i]);
+ for (String v : valuesArray[i]) {
+ groupValues.add(v);
+ }
+ }
+ });
+ }
+
+ @Override
+ public void aggregateGroupByMV(int length, int[][] groupKeysArray,
GroupByResultHolder groupByResultHolder,
+ Map<ExpressionContext, BlockValSet> blockValSetMap) {
+ BlockValSet blockValSet = blockValSetMap.get(_expression);
+ String[][] valuesArray = blockValSet.getStringValuesMV();
+ forEachNotNull(length, blockValSet, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ int[] groupKeys = groupKeysArray[i];
+ for (int groupKey : groupKeys) {
+ ObjectCollection<String> groupValues =
getObjectCollection(groupByResultHolder, groupKey);
+ for (String v : valuesArray[i]) {
+ groupValues.add(v);
+ }
+ }
+ }
+ });
+ }
+
+ @Override
+ public ObjectCollection<String>
extractAggregationResult(AggregationResultHolder aggregationResultHolder) {
+ return aggregationResultHolder.getResult();
+ }
+
+ @Override
+ public ObjectCollection<String> extractGroupByResult(GroupByResultHolder
groupByResultHolder, int groupKey) {
+ return groupByResultHolder.getResult(groupKey);
+ }
+
+ @Override
+ public ObjectCollection<String> merge(ObjectCollection<String>
intermediateResult1,
+ ObjectCollection<String> intermediateResult2) {
+ if (intermediateResult1 == null) {
+ return intermediateResult2;
+ }
+ if (intermediateResult2 == null) {
+ return intermediateResult1;
+ }
+ intermediateResult1.addAll(intermediateResult2);
+ return intermediateResult1;
+ }
+
+ @Override
+ public ColumnDataType getIntermediateResultColumnType() {
+ return ColumnDataType.OBJECT;
+ }
+
+ @Override
+ public SerializedIntermediateResult
serializeIntermediateResult(ObjectCollection<String> strings) {
+ ObjectArrayList<String> list = new ObjectArrayList<>();
+ for (String s : strings) {
+ list.add(s);
+ }
+ return new
SerializedIntermediateResult(ObjectSerDeUtils.ObjectType.StringArrayList.getValue(),
+ ObjectSerDeUtils.STRING_ARRAY_LIST_SER_DE.serialize(list));
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public ObjectCollection<String> deserializeIntermediateResult(CustomObject
customObject) {
+ ObjectArrayList<String> list =
ObjectSerDeUtils.STRING_ARRAY_LIST_SER_DE.deserialize(customObject.getBuffer());
+ return list;
Review Comment:
The local variable `list` is unnecessary. Return the deserialized result
directly: `return
ObjectSerDeUtils.STRING_ARRAY_LIST_SER_DE.deserialize(customObject.getBuffer());`
```suggestion
return
ObjectSerDeUtils.STRING_ARRAY_LIST_SER_DE.deserialize(customObject.getBuffer());
```
--
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]
