Copilot commented on code in PR #19302:
URL: https://github.com/apache/pinot/pull/19302#discussion_r3809348131
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunction.java:
##########
@@ -105,6 +105,15 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
DataType storedType = dataType.getStoredType();
+ if (blockValSet.isSingleValue()) {
+ aggregateSV(length, aggregationResultHolder, blockValSet, storedType);
+ } else {
+ aggregateMV(length, aggregationResultHolder, blockValSet, storedType);
+ }
Review Comment:
This query-semantics change is covered only by direct aggregator unit calls,
so it does not verify planner-to-segment dispatch with an actual MV forward
index. `UuidAggregationTest` already provides dictionary and raw MV columns and
currently excludes MV from its `DISTINCTCOUNTULL` query; extend that
integration test to query both MV columns (including grouped execution).
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/DistinctCountULLAggregationFunctionTest.java:
##########
@@ -34,28 +42,82 @@ public void testCanUseStarTreeDefaultP() {
DistinctCountULLAggregationFunction function = new
DistinctCountULLAggregationFunction(
List.of(ExpressionContext.forIdentifier("col")));
- Assert.assertTrue(function.canUseStarTree(Map.of()));
-
Assert.assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"12")));
-
Assert.assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
12)));
-
Assert.assertFalse(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
16)));
+ assertTrue(function.canUseStarTree(Map.of()));
+ assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"12")));
+ assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
12)));
+ assertFalse(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
16)));
function = new
DistinctCountULLAggregationFunction(List.of(ExpressionContext.forIdentifier("col"),
ExpressionContext.forLiteral(Literal.intValue(12))));
- Assert.assertTrue(function.canUseStarTree(Map.of()));
-
Assert.assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"12")));
-
Assert.assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
12)));
-
Assert.assertFalse(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"16")));
+ assertTrue(function.canUseStarTree(Map.of()));
+ assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"12")));
+ assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
12)));
+ assertFalse(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"16")));
}
@Test
public void testCanUseStarTreeCustomP() {
DistinctCountULLAggregationFunction function = new
DistinctCountULLAggregationFunction(
List.of(ExpressionContext.forIdentifier("col"),
ExpressionContext.forLiteral(Literal.stringValue("16"))));
- Assert.assertFalse(function.canUseStarTree(Map.of()));
-
Assert.assertFalse(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"12")));
-
Assert.assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
16)));
-
Assert.assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"16")));
+ assertFalse(function.canUseStarTree(Map.of()));
+ assertFalse(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"12")));
+ assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
16)));
+ assertTrue(function.canUseStarTree(Map.of(Constants.HLLPLUS_ULL_P_KEY,
"16")));
+ }
+
+ private static final ExpressionContext COLUMN =
ExpressionContext.forIdentifier("column");
+ private static final long[][] MV_ROWS = {{1L, 2L}, {3L, 4L}, {5L, 6L}, {1L,
3L}};
+ private static final long[] FLATTENED = {1L, 2L, 3L, 4L, 5L, 6L, 1L, 3L};
+
+ private static DistinctCountULLAggregationFunction create() {
+ return new DistinctCountULLAggregationFunction(List.of(COLUMN));
+ }
+
+ private static Map<ExpressionContext, BlockValSet> mvBlock() {
+ return Map.of(COLUMN, SyntheticBlockValSets.LongMV.create(null, MV_ROWS));
Review Comment:
The new tests only exercise the non-dictionary `LONG` branch, while this
change adds separate dictionary handling and six stored-type cases. A broken
`getDictionaryIdsMV()` path or any of the other five accessors would therefore
go undetected. Please add a TestNG data provider/real fixtures covering
dictionary-encoded MV input and each supported stored type across the three
entry points.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SegmentPartitionedDistinctCountAggregationFunction.java:
##########
@@ -72,7 +72,14 @@ public GroupByResultHolder createGroupByResultHolder(int
initialCapacity, int ma
public void aggregate(int length, AggregationResultHolder
aggregationResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
BlockValSet blockValSet = blockValSetMap.get(_expression);
+ if (blockValSet.isSingleValue()) {
+ aggregateSV(length, aggregationResultHolder, blockValSet);
+ } else {
+ aggregateMV(length, aggregationResultHolder, blockValSet);
Review Comment:
The new MV dispatch is tested only by calling this class directly. Add a
custom-cluster integration query over a real MV column for aggregate,
group-by-SV, and group-by-MV shapes; otherwise reader/planner integration
regressions can pass these unit tests while the SQL feature still fails end to
end.
This issue also appears on line 75 of the same file.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/SegmentPartitionedDistinctCountAggregationFunctionTest.java:
##########
@@ -0,0 +1,98 @@
+/**
+ * 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;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.request.context.ExpressionContext;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.SyntheticBlockValSets;
+import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import
org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertEquals;
+
+
+/// Multi-value column support for `SEGMENTPARTITIONEDDISTINCTCOUNT`.
+///
+/// This function used to read the single-value accessors unconditionally, so
a multi-value column was not something
+/// it could aggregate at all. It now branches on
`BlockValSet.isSingleValue()` the way the HyperLogLog and bitmap
+/// families already did. Its counts are exact, so the answers are asserted
directly rather than as estimates.
+public class SegmentPartitionedDistinctCountAggregationFunctionTest {
+ private static final ExpressionContext COLUMN =
ExpressionContext.forIdentifier("column");
+ private static final long[][] MV_ROWS = {{1L, 2L}, {3L, 4L}, {5L, 6L}, {1L,
3L}};
+ private static final long[] FLATTENED = {1L, 2L, 3L, 4L, 5L, 6L, 1L, 3L};
+
+ private static SegmentPartitionedDistinctCountAggregationFunction create() {
+ return new
SegmentPartitionedDistinctCountAggregationFunction(List.of(COLUMN));
+ }
+
+ private static Map<ExpressionContext, BlockValSet> mvBlock() {
+ return Map.of(COLUMN, SyntheticBlockValSets.LongMV.create(null, MV_ROWS));
Review Comment:
This fixture covers only the non-dictionary `LONG`/hash-set branch. The
implementation has materially different dictionary and `INT` bitmap paths plus
FLOAT, DOUBLE, STRING, and BYTES sets, so errors in those new branches are
currently invisible. Please make these tests data-driven over all supported
stored types and include a real dictionary-encoded MV case for each aggregation
shape.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SegmentPartitionedDistinctCountAggregationFunction.java:
##########
@@ -298,6 +491,97 @@ public void aggregateGroupByMV(int length, int[][]
groupKeysArray, GroupByResult
}
}
+ protected void aggregateMVGroupByMV(int length, int[][] groupKeysArray,
GroupByResultHolder groupByResultHolder,
+ BlockValSet blockValSet) {
+ // For dictionary-encoded expression, store dictionary ids into a
RoaringBitmap
+ if (blockValSet.isDictionaryEncoded()) {
+ int[][] dictIds = blockValSet.getDictionaryIdsMV();
+ for (int i = 0; i < length; i++) {
+ int[] rowDictIds = dictIds[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (int dictId : rowDictIds) {
+ setIntValueForGroup(groupByResultHolder, groupKey, dictId);
+ }
+ }
+ }
+ return;
+ }
+
+ // For non-dictionary-encoded expression, store INT values into a
RoaringBitmap, other types into an OpenHashSet
+ DataType storedType = blockValSet.getValueType().getStoredType();
+ switch (storedType) {
+ case INT:
+ int[][] intValues = blockValSet.getIntValuesMV();
+ for (int i = 0; i < length; i++) {
+ int[] intRow = intValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (int value : intRow) {
+ setIntValueForGroup(groupByResultHolder, groupKey, value);
+ }
+ }
+ }
+ break;
+ case LONG:
+ long[][] longValues = blockValSet.getLongValuesMV();
+ for (int i = 0; i < length; i++) {
+ long[] longRow = longValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (long value : longRow) {
+ setLongValueForGroup(groupByResultHolder, groupKey, value);
+ }
+ }
+ }
+ break;
+ case FLOAT:
+ float[][] floatValues = blockValSet.getFloatValuesMV();
+ for (int i = 0; i < length; i++) {
+ float[] floatRow = floatValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (float value : floatRow) {
+ setFloatValueForGroup(groupByResultHolder, groupKey, value);
+ }
+ }
+ }
+ break;
+ case DOUBLE:
+ double[][] doubleValues = blockValSet.getDoubleValuesMV();
+ for (int i = 0; i < length; i++) {
+ double[] doubleRow = doubleValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (double value : doubleRow) {
+ setDoubleValueForGroup(groupByResultHolder, groupKey, value);
+ }
+ }
+ }
+ break;
+ case STRING:
+ String[][] stringValues = blockValSet.getStringValuesMV();
+ for (int i = 0; i < length; i++) {
+ String[] stringRow = stringValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (String value : stringRow) {
+ setStringValueForGroup(groupByResultHolder, groupKey, value);
+ }
+ }
+ }
+ break;
+ case BYTES:
+ byte[][][] bytesValues = blockValSet.getBytesValuesMV();
+ for (int i = 0; i < length; i++) {
+ byte[][] bytesRow = bytesValues[i];
+ for (int groupKey : groupKeysArray[i]) {
+ for (byte[] value : bytesRow) {
+ setBytesValueForGroup(groupByResultHolder, groupKey, new
ByteArray(value));
+ }
+ }
+ }
Review Comment:
This allocates a new `ByteArray` for every `(groupKey, value)` pair. For MV
group-by rows, allocation grows as `groups × byte values`, although the same
immutable wrapper can be shared across all groups (as the SV helper already
does). Iterate `bytesRow` first, create one wrapper per value, then add it to
each group.
--
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]
