yashmayya commented on code in PR #14071:
URL: https://github.com/apache/pinot/pull/14071#discussion_r1778020911
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -145,12 +145,23 @@ public void aggregateGroupBySV(int length, int[]
groupKeyArray, GroupByResultHol
@Override
public void aggregateGroupByMV(int length, int[][] groupKeysArray,
GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
- if (blockValSetMap.isEmpty() ||
!blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {
Review Comment:
The check was added
[here](https://github.com/apache/pinot/pull/9086/files#diff-1d2ce5c094a4f0288a67ef21be77efc045f3997a324e01df8fe7f4ac56110702R161),
only for additional clarity I presume. For the count aggregation function,
there's two cases where the `blockValSetMap` passed in to the `aggregate` /
`aggregateGroupBySV` / `aggregateGroupByMV` will be non-empty and one case
where it will be empty.
- If null handling is not enabled and there is no star-tree index available
for the function / column pair, the map will be empty - see
[here](https://github.com/apache/pinot/blob/bba61eef14a49e7ed7a5c4e73c640c12b916a5d6/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java#L152-L153),
[here](https://github.com/apache/pinot/blob/7668b212bb44182779ded3c2d95b158cda886bc3/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java#L113-L117),
[here](https://github.com/apache/pinot/blob/7668b212bb44182779ded3c2d95b158cda886bc3/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java#L70).
One important thing to note here is that null handling will be considered
"disabled" for the function even if it is actually enabled but the expression
is `COUNT(*)` or `COUNT('literal')` due to the semantics for those aggregation
functions (where even `null` will
be "counted") -
https://github.com/apache/pinot/blob/bba61eef14a49e7ed7a5c4e73c640c12b916a5d6/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java#L45-L51
- If null handling is enabled (`COUNT(col)` with `enableNullHandling=true`),
then the `blockValSetMap` will be non-empty since the expression
[here](https://github.com/apache/pinot/blob/7668b212bb44182779ded3c2d95b158cda886bc3/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java#L118-L121)
will be `col` (the column being aggregated).
- If the star-tree index based aggregation / group by executor is being
used, then the `blockValSetMap` will be non-empty with the single
`ExpressionContext` key being `*` regardless of the actual expression used in
`COUNT` (see
[here](https://github.com/apache/pinot/blob/bba61eef14a49e7ed7a5c4e73c640c12b916a5d6/pinot-core/src/main/java/org/apache/pinot/core/startree/executor/StarTreeGroupByExecutor.java#L76-L77),
[here](https://github.com/apache/pinot/blob/bba61eef14a49e7ed7a5c4e73c640c12b916a5d6/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java#L75-L77)).
This is because the star-tree index doesn't support null handling and will
only be used when null handling is disabled.
So the three if else blocks here cover all the possible cases and is the
same pattern followed in the `aggregate` and `aggregateGroupBySV` methods -
https://github.com/apache/pinot/blob/bba61eef14a49e7ed7a5c4e73c640c12b916a5d6/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java#L84-L107
https://github.com/apache/pinot/blob/bba61eef14a49e7ed7a5c4e73c640c12b916a5d6/pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java#L110-L143
--
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]
