dario-liberman commented on code in PR #11092:
URL: https://github.com/apache/pinot/pull/11092#discussion_r1272679653
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/FunnelCountAggregationFunction.java:
##########
@@ -109,47 +185,52 @@ public GroupByResultHolder createGroupByResultHolder(int
initialCapacity, int ma
@Override
public void aggregate(int length, AggregationResultHolder
aggregationResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
- getAggregationStrategyByBlockValSetMap(blockValSetMap).aggregate(length,
aggregationResultHolder, blockValSetMap);
+ getAggregationStrategy(blockValSetMap)
+ .aggregate(length, aggregationResultHolder, blockValSetMap);
}
@Override
public void aggregateGroupBySV(int length, int[] groupKeyArray,
GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
-
getAggregationStrategyByBlockValSetMap(blockValSetMap).aggregateGroupBySV(length,
groupKeyArray,
- groupByResultHolder, blockValSetMap);
+ getAggregationStrategy(blockValSetMap)
+ .aggregateGroupBySV(length, groupKeyArray, groupByResultHolder,
blockValSetMap);
}
@Override
public void aggregateGroupByMV(int length, int[][] groupKeysArray,
GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
-
getAggregationStrategyByBlockValSetMap(blockValSetMap).aggregateGroupByMV(length,
groupKeysArray,
- groupByResultHolder, blockValSetMap);
+ getAggregationStrategy(blockValSetMap)
+ .aggregateGroupByMV(length, groupKeysArray, groupByResultHolder,
blockValSetMap);
}
@Override
- public List<Long> extractAggregationResult(AggregationResultHolder
aggregationResultHolder) {
- return
getAggregationStrategyByAggregationResult(aggregationResultHolder.getResult()).extractAggregationResult(
- aggregationResultHolder);
+ public Object extractAggregationResult(AggregationResultHolder
aggregationResultHolder) {
+ return getResultExtractionStrategy(aggregationResultHolder.getResult())
+ .extractAggregationResult(aggregationResultHolder);
}
-
@Override
- public List<Long> extractGroupByResult(GroupByResultHolder
groupByResultHolder, int groupKey) {
- return
getAggregationStrategyByAggregationResult(groupByResultHolder.getResult(groupKey)).extractGroupByResult(
- groupByResultHolder, groupKey);
+ public Object extractGroupByResult(GroupByResultHolder groupByResultHolder,
int groupKey) {
+ return getResultExtractionStrategy(groupByResultHolder.getResult(groupKey))
+ .extractGroupByResult(groupByResultHolder, groupKey);
}
@Override
- public List<Long> merge(List<Long> a, List<Long> b) {
- int length = a.size();
- Preconditions.checkState(length == b.size(), "The two operand arrays are
not of the same size! provided %s, %s",
- length, b.size());
+ public Object merge(Object a, Object b) {
Review Comment:
It can be a wrapper type, but it would not be known to the underlying merge
strategy, I don't think, as each uses a different aggregation type.
I personally think that the abstraction is unnecessary and will create
unnecessary garbage collection burden.
Note also that although the `AggregationFunction` interface has a type
parameter for the intermediate result, everything outside just uses `Object`,
see for example `IndexedTable`.
I considered to propagate the type further up to avoid the use of `Object`
here, using a generic type instead, moving some of the strategy selection to a
separate aggregation function factory class.
There are two main challenges in that approach: (1) segments might be
unsorted, making strategy selection dynamic. (2) currently it supports quite a
few combinations of strategies (but I can probably make it more dumb and
support specific combinations for the sake of readability/maintainability).
--
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]
