Jackie-Jiang commented on code in PR #18873:
URL: https://github.com/apache/pinot/pull/18873#discussion_r3787064505
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java:
##########
@@ -820,9 +825,14 @@ private static HyperLogLog
getDistinctCountHLLResult(Dictionary dictionary,
}
}
- private static HyperLogLogPlus getDistinctCountHLLPlusResult(Dictionary
dictionary,
+ private static HyperLogLogPlus getDistinctCountHLLPlusResult(DataSource
dataSource,
DistinctCountHLLPlusAggregationFunction function, String
explainPlanName) {
- if (dictionary.getValueType() == FieldSpec.DataType.BYTES) {
+ Dictionary dictionary = Objects.requireNonNull(dataSource.getDictionary());
+ // A UUID column's dictionary reports BYTES (it is a plain
BytesDictionary), but its entries are logical
+ // scalars, not serialized sketch state. Excluding it here lets it fall
through to the scalar path
+ // below, which offers dictionary.get(i) -- the stored byte[] -- exactly
as the scan path does.
+ if (dataSource.getDataSourceMetadata().getDataType() !=
FieldSpec.DataType.UUID
+ && dictionary.getValueType() == FieldSpec.DataType.BYTES) {
Review Comment:
Same for other places
```suggestion
if (dataSource.getDataSourceMetadata().getDataType() ==
FieldSpec.DataType.BYTES) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapAggregationFunction.java:
##########
@@ -71,9 +72,11 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
Map<ExpressionContext, BlockValSet> blockValSetMap) {
BlockValSet blockValSet = blockValSetMap.get(_expression);
+ DataType dataType = blockValSet.getValueType();
+ DataType storedType = dataType.getStoredType();
+
// Treat BYTES value as serialized RoaringBitmap
- DataType storedType = blockValSet.getValueType().getStoredType();
- if (storedType == DataType.BYTES) {
+ if (storedType == DataType.BYTES && dataType != DataType.UUID) {
Review Comment:
Same for other places
```suggestion
DataType dataType = blockValSet.getValueType();
// Treat BYTES value as serialized RoaringBitmap
if (dataType == DataType.BYTES) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapAggregationFunction.java:
##########
@@ -138,6 +141,13 @@ protected void aggregateSV(int length,
AggregationResultHolder aggregationResult
valueBitmap.add(stringValues[i].hashCode());
}
break;
+ // Reached only by UUID: a real BYTES column is serialized sketch state
and is handled above.
Review Comment:
(minor) Don't add this comment. We can add more logical types in the future
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapAggregationFunction.java:
##########
@@ -198,6 +208,15 @@ protected void aggregateMV(int length,
AggregationResultHolder aggregationResult
}
}
break;
+ // Reached only by UUID: a real BYTES column is serialized sketch state
and is handled above.
Review Comment:
This is wrong. MV BYTES is handled here
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapAggregationFunction.java:
##########
@@ -138,6 +141,13 @@ protected void aggregateSV(int length,
AggregationResultHolder aggregationResult
valueBitmap.add(stringValues[i].hashCode());
}
break;
+ // Reached only by UUID: a real BYTES column is serialized sketch state
and is handled above.
+ case BYTES:
+ byte[][] uuidValues = blockValSet.getBytesValuesSV();
Review Comment:
(minor) Rename it to `bytesValues` for future compatibility
--
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]
