Jackie-Jiang commented on code in PR #18144:
URL: https://github.com/apache/pinot/pull/18144#discussion_r3121297286
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountHLLAggregationFunction.java:
##########
@@ -630,16 +638,30 @@ public boolean canUseStarTree(Map<String, Object>
functionParameters) {
}
/**
- * Returns the dictionary id bitmap from the result holder or creates a new
one if it does not exist.
+ * Returns the {@link GroupByDictIdsWrapper} for the given group key,
creating a new one if absent.
+ * Uses a {@link RoaringBitmap} (sparse) so memory scales with distinct
values per group, not dictionary size.
*/
- protected static RoaringBitmap getDictIdBitmap(AggregationResultHolder
aggregationResultHolder,
+ protected static GroupByDictIdsWrapper getDictIdBitmap(GroupByResultHolder
groupByResultHolder, int groupKey,
+ Dictionary dictionary) {
+ GroupByDictIdsWrapper wrapper = groupByResultHolder.getResult(groupKey);
+ if (wrapper == null) {
+ wrapper = new GroupByDictIdsWrapper(dictionary);
+ groupByResultHolder.setValueForKey(groupKey, wrapper);
+ }
+ return wrapper;
+ }
+
+ /**
+ * Returns the {@link DictIdsWrapper} from the result holder, creating a new
one if absent.
+ */
+ protected static DictIdsWrapper getDictIdBitSet(AggregationResultHolder
aggregationResultHolder,
Review Comment:
Consider directly returning the `BitSet` to align with the method name and
be more explicit
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountHLLAggregationFunction.java:
##########
@@ -630,16 +638,30 @@ public boolean canUseStarTree(Map<String, Object>
functionParameters) {
}
/**
- * Returns the dictionary id bitmap from the result holder or creates a new
one if it does not exist.
+ * Returns the {@link GroupByDictIdsWrapper} for the given group key,
creating a new one if absent.
+ * Uses a {@link RoaringBitmap} (sparse) so memory scales with distinct
values per group, not dictionary size.
*/
- protected static RoaringBitmap getDictIdBitmap(AggregationResultHolder
aggregationResultHolder,
+ protected static GroupByDictIdsWrapper getDictIdBitmap(GroupByResultHolder
groupByResultHolder, int groupKey,
Review Comment:
Consider directly returning the `RoaringBitmap` to align with the method
name and be more explicit
--
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]
