Jackie-Jiang commented on code in PR #19302:
URL: https://github.com/apache/pinot/pull/19302#discussion_r3810865631
##########
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:
Fixed. The wrapper is now created once per value and shared across that
row's group keys, matching what the single-value helper a few lines up already
did:
```java
for (byte[] value : bytesValues[i]) {
ByteArray byteArray = new ByteArray(value);
for (int groupKey : groupKeysArray[i]) {
setBytesValueForGroup(groupByResultHolder, groupKey, byteArray);
}
}
```
Checking for the same shape across the rest of the aggregation functions
turned up two more pre-existing instances, which I am leaving for a separate PR
since they are unrelated files: `BaseDistinctAggregateAggregationFunction`
(same `ByteArray` case) and `SumPrecisionAggregationFunction`, where the
repeated call is `new BigDecimal(String)` — a parse rather than just an
allocation, so more costly per group.
##########
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:
Done, and thanks for the pointer to `UuidAggregationTest` — that comment at
line 171 saying `DISTINCTCOUNTULL currently supports only single-value inputs`
was made false by this change, which I had missed.
`DISTINCTCOUNTULL` now runs in the same loop as the other distinct-count
functions over the dictionary-encoded SV, raw SV and raw MV columns, and the
special-cased single-value-only query and its comment are gone. I added a
separate case for the dictionary-encoded MV column, since that path collects
dictionary ids and resolves them at extract time rather than reading values, so
the raw MV column does not cover it.
--
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]
