[GitHub] [pinot] nizarhejazi commented on a diff in pull request #9086: Proper null handling in Aggregation functions for SV data types

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

nizarhejazi commented on code in PR #9086:
URL: https://github.com/apache/pinot/pull/9086#discussion_r929689219


##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MaxAggregationFunction.java:
##########
@@ -28,13 +28,19 @@
 import org.apache.pinot.core.query.aggregation.groupby.DoubleGroupByResultHolder;
 import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
 import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.roaringbitmap.RoaringBitmap;
 
 
 public class MaxAggregationFunction extends BaseSingleInputAggregationFunction<Double, Double> {
   private static final double DEFAULT_INITIAL_VALUE = Double.NEGATIVE_INFINITY;
+  private final boolean _nullHandlingEnabled;
 
-  public MaxAggregationFunction(ExpressionContext expression) {
+  // stores id of the groupKey where the corresponding value is null.
+  private Integer _groupKeyForNullValue = null;

Review Comment:
   @Jackie-Jiang The problem with tracking `groupKeyForNullValue` in `GroupByExecutor` is the following:
   - In `MaxAggregationFunction`, `DEFAULT_INITIAL_VALUE` is `Double.NEGATIVE_INFINITY`. `aggregateGroupBySV()` calls `setValueForKey` only if the value > `DEFAULT_INITIAL_VALUE` (or the latest set value). `DEFAULT_INITIAL_VALUE` could be a valid non-null value or a null value (e.g. dimension column). If `nullHandlingEnabled` is set to true, we should not run this check (value > `DEFAULT_INITIAL_VALUE`). In summary, if `nullHandlingEnabled` is set to true, the whole range of values in a data type is a valid range including `Double.NEGATIVE_INFINITY`.
   - The same applies to `MinAggregationFunction`, and the `DEFAULT_VALUE` = `Double.POSITIVE_INFINITY`.
   



-- 
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