xiangfu0 commented on code in PR #18873:
URL: https://github.com/apache/pinot/pull/18873#discussion_r3787530621
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountBitmapAggregationFunction.java:
##########
@@ -71,8 +72,22 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
Map<ExpressionContext, BlockValSet> blockValSetMap) {
BlockValSet blockValSet = blockValSetMap.get(_expression);
+ DataType dataType = blockValSet.getValueType();
+ DataType storedType = dataType.getStoredType();
+
+ // UUID values are logical scalars (stored as 16-byte BYTES) — not
serialized RoaringBitmap state. Add the
+ // hashCode of the canonical UUID string so DISTINCTCOUNTBITMAP(uuidCol)
matches
+ // DISTINCTCOUNTBITMAP(CAST(uuidCol AS STRING)).
+ if (dataType == DataType.UUID) {
Review Comment:
Addressed in 938dda3b4c. The stored-BYTES handling now lives in the
cardinality-specific switch branches across the affected aggregation functions.
SV logical BYTES remains serialized sketch state; functions with MV support use
their MV raw-value accessors for MV BYTES and UUID. Added aggregate,
group-by-SV, and group-by-MV regressions plus UUID ingestion/query coverage.
Focused core tests: 70 passed; UuidAggregationTest and CpcSketchTest: 11 passed.
##########
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:
Updated all HLL, HLLPlus, and ULL dictionary fast paths to use the logical
metadata type. The additional isSingleValue guard is intentional: SV logical
BYTES is serialized sketch state, while MV logical BYTES is handled as raw
values by the MV functions. Added dictionary-path tests for both cases.
##########
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:
Updated all corresponding entry points to use the logical DataType and moved
the serialized-versus-raw decision into the SV case BYTES branches. This keeps
MV BYTES on the raw MV path and also supports future logical types backed by
BYTES.
##########
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:
Fixed. MV case BYTES remains on the raw-value path and uses
getBytesValuesMV; only SV logical BYTES is deserialized as sketch state. Added
aggregate, group-by-SV, and group-by-MV tests that verify the MV accessor is
used and the SV accessor is not.
##########
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:
Removed the UUID-only comments from all affected aggregation functions so
the branches remain compatible with future logical types backed by 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.
+ case BYTES:
+ byte[][] uuidValues = blockValSet.getBytesValuesSV();
Review Comment:
Renamed the UUID-specific locals to bytesValues or bytesValuesArray across
the affected aggregation functions.
--
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]
