xiangfu0 commented on code in PR #18873:
URL: https://github.com/apache/pinot/pull/18873#discussion_r3753597310
##########
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:
You are right, and it is worse than one function — thanks for catching it.
I checked all of them: `DistinctCountHLL`, `DistinctCountHLLPlus`,
`DistinctCountCPCSketch`, `DistinctCountULL` and `DistinctCountBitmap` each
have 3 UUID branches (aggregate, aggregateGroupBySV, aggregateGroupByMV) and
**none** of them reference `getBytesValuesMV` — 15 branches, all calling
`getBytesValuesSV()` unconditionally before the `isSingleValue()` dispatch. So
an MV UUID column takes the SV accessor in every case.
`DistinctCountThetaSketch` is the only one that handles MV, because its branch
sits after the SV/MV split.
Your point about MV BYTES is also right and predates this PR: the
`storedType == BYTES` block has the same placement, above the dispatch.
I have not fixed it yet — restructuring 15 branches into `aggregateSV()` /
`aggregateMV()` across five files is more than I want to push without running
the full suite behind it, so I would rather do it as a focused follow-up commit
than a rushed one. Leaving this thread open until it is done.
One question on scope while I am in there: do you want the pre-existing MV
BYTES gap fixed in the same pass, or kept separate so this PR stays UUID-only?
Fixing it changes behaviour for existing BYTES columns, which felt like it
deserved its own change, but it is the same few lines.
--
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]
