[GitHub] [pinot] siddharthteotia commented on a change in pull request #8139: Aggregation delay conversion to double

[GitBox <gi...@apache.org>]

siddharthteotia commented on a change in pull request #8139:
URL: https://github.com/apache/pinot/pull/8139#discussion_r800103629



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MaxAggregationFunction.java
##########
@@ -54,13 +54,39 @@ public GroupByResultHolder createGroupByResultHolder(int initialCapacity, int ma
   @Override
   public void aggregate(int length, AggregationResultHolder aggregationResultHolder,
       Map<ExpressionContext, BlockValSet> blockValSetMap) {
-    double[] valueArray = blockValSetMap.get(_expression).getDoubleValuesSV();
     double max = aggregationResultHolder.getDoubleResult();
-    for (int i = 0; i < length; i++) {
-      double value = valueArray[i];
-      if (value > max) {
-        max = value;
+    BlockValSet blockValSet = blockValSetMap.get(_expression);

Review comment:
       I understand how delaying conversion to double will help with heap usage. Couple of questions on performance:
   
   - Now every aggregate() call has to execute switch condition once. Will there be any perf penalty for this ?
   - Why does conversion to double[] prevent auto-vectorizatonn ?




-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org