Jackie-Jiang commented on code in PR #19302:
URL: https://github.com/apache/pinot/pull/19302#discussion_r3810868285
##########
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:
Partly addressed. Both test classes now have a dictionary-encoded MV case,
which was the branch I most agreed was missing — it collects dictionary ids and
resolves them only at extract time, so nothing else covered it. I
mutation-checked both: changing `dictIds[i]` to `dictIds[i][0]` in the MV
dictionary path makes them fail 6 -> 3, so they exercise the branch rather than
passing vacuously.
I have not made them data-driven over all six stored types, and want to be
explicit that this is a judgement call rather than an oversight. The per-type
MV bodies are mechanically identical apart from the accessor and element type,
and covering them needs four more MV fixtures in `SyntheticBlockValSets`
(`IntMV`, `FloatMV`/`DoubleMV`, `StrMV`, `BytesMV`) that nothing else uses
today. End-to-end coverage now comes from `UuidAggregationTest`, which
exercises the `BYTES` stored type over both dictionary-encoded and raw MV
columns. Happy to add the full matrix if a maintainer would rather have it.
##########
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:
Partly addressed. Both test classes now have a dictionary-encoded MV case,
which was the branch I most agreed was missing — it collects dictionary ids and
resolves them only at extract time, so nothing else covered it. I
mutation-checked both: changing `dictIds[i]` to `dictIds[i][0]` in the MV
dictionary path makes them fail 6 -> 3, so they exercise the branch rather than
passing vacuously.
I have not made them data-driven over all six stored types, and want to be
explicit that this is a judgement call rather than an oversight. The per-type
MV bodies are mechanically identical apart from the accessor and element type,
and covering them needs four more MV fixtures in `SyntheticBlockValSets`
(`IntMV`, `FloatMV`/`DoubleMV`, `StrMV`, `BytesMV`) that nothing else uses
today. End-to-end coverage now comes from `UuidAggregationTest`, which
exercises the `BYTES` stored type over both dictionary-encoded and raw MV
columns. Happy to add the full matrix if a maintainer would rather have it.
##########
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:
I tried this and then backed it out, so recording the reason rather than
silently declining.
Adding `SEGMENTPARTITIONEDDISTINCTCOUNT` to `UuidAggregationTest` does pass
— the fixture overrides `getNumAvroFiles()` to 1, so there is a single segment
and the per-segment sum equals an exact distinct count. But the function's
contract requires values to be non-overlapping across segments, and this
fixture violates that: `UUID_0` appears twice in the single-value column and in
two separate multi-value rows. The assertion would therefore encode a
coincidence of the segment layout, and would start failing if anyone changed
that override to 2 — for a reason having nothing to do with multi-value support.
Its MV paths are covered directly by the new unit test instead, including
the dictionary-id branch. An integration test that genuinely exercises this
function wants a fixture built to satisfy its partitioning premise, which seems
better as its own change than bolted onto the UUID test.
--
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]
