Jackie-Jiang commented on code in PR #9086:
URL: https://github.com/apache/pinot/pull/9086#discussion_r937026972
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumAggregationFunction.java:
##########
@@ -102,10 +130,112 @@ public void aggregate(int length,
AggregationResultHolder aggregationResultHolde
aggregationResultHolder.setValue(sum);
}
+ private void aggregateNullHandlingEnabled(int length,
AggregationResultHolder aggregationResultHolder,
+ BlockValSet blockValSet, RoaringBitmap nullBitmap) {
+ double sum = 0;
+ switch (blockValSet.getValueType().getStoredType()) {
+ case INT: {
+ int[] values = blockValSet.getIntValuesSV();
+ if (nullBitmap.getCardinality() < Math.min(length, values.length)) {
Review Comment:
(minor) We don't need to compute `Math.min(length, values.length)` because
`values` is a value buffer, which always have `values.length >= length`. Same
for other places
```suggestion
if (nullBitmap.getCardinality() < length) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java:
##########
@@ -101,7 +112,17 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
}
break;
case FLOAT:
+ float[] floatValues = blockValSet.getFloatValuesSV();
+ for (int i = 0; i < length; i++) {
+ sum = sum.add(BigDecimal.valueOf(floatValues[i]));
+ }
+ break;
case DOUBLE:
+ double[] doubleValues = blockValSet.getDoubleValuesSV();
+ for (int i = 0; i < length; i++) {
+ sum = sum.add(BigDecimal.valueOf(doubleValues[i]));
+ }
+ break;
Review Comment:
(minor) We don't need this change because `BigDecimal.valueOf(double)` will
first convert double value to string
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumAggregationFunction.java:
##########
@@ -102,10 +130,112 @@ public void aggregate(int length,
AggregationResultHolder aggregationResultHolde
aggregationResultHolder.setValue(sum);
}
+ private void aggregateNullHandlingEnabled(int length,
AggregationResultHolder aggregationResultHolder,
+ BlockValSet blockValSet, RoaringBitmap nullBitmap) {
+ double sum = 0;
+ switch (blockValSet.getValueType().getStoredType()) {
+ case INT: {
+ int[] values = blockValSet.getIntValuesSV();
+ if (nullBitmap.getCardinality() < Math.min(length, values.length)) {
Review Comment:
Move value read into the if block to avoid unnecessary value read. Same for
other places
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumAggregationFunction.java:
##########
@@ -19,22 +19,36 @@
package org.apache.pinot.core.query.aggregation.function;
import java.math.BigDecimal;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import org.apache.pinot.common.request.context.ExpressionContext;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.common.BlockValSet;
import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
import org.apache.pinot.core.query.aggregation.DoubleAggregationResultHolder;
+import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
import
org.apache.pinot.core.query.aggregation.groupby.DoubleGroupByResultHolder;
import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
+import
org.apache.pinot.core.query.aggregation.groupby.ObjectGroupByResultHolder;
import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.roaringbitmap.RoaringBitmap;
public class SumAggregationFunction extends
BaseSingleInputAggregationFunction<Double, Double> {
private static final double DEFAULT_VALUE = 0.0;
+ private final boolean _nullHandlingEnabled;
+
+ private final Set<Integer> _groupKeysWithNonNullValue;
Review Comment:
This is still not fixed
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java:
##########
@@ -126,10 +147,120 @@ public void aggregate(int length,
AggregationResultHolder aggregationResultHolde
aggregationResultHolder.setValue(sum);
}
+ private void aggregateNullHandlingEnabled(int length,
AggregationResultHolder aggregationResultHolder,
+ BlockValSet blockValSet, RoaringBitmap nullBitmap) {
+ BigDecimal sum = BigDecimal.ZERO;
+ switch (blockValSet.getValueType().getStoredType()) {
+ case INT: {
+ int[] intValues = blockValSet.getIntValuesSV();
+ if (nullBitmap.getCardinality() < length) {
+ // TODO: need to update the for-loop terminating condition to: i <
length & i < intValues.length?
+ for (int i = 0; i < length; i++) {
+ if (!nullBitmap.contains(i)) {
+ sum = sum.add(BigDecimal.valueOf(intValues[i]));
+ }
+ }
+ setAggregationResult(aggregationResultHolder, sum);
+ }
+ break;
+ }
+ case LONG: {
+ long[] longValues = blockValSet.getLongValuesSV();
+ if (nullBitmap.getCardinality() < length) {
+ for (int i = 0; i < length; i++) {
+ if (!nullBitmap.contains(i)) {
+ sum = sum.add(BigDecimal.valueOf(longValues[i]));
+ }
+ }
+ setAggregationResult(aggregationResultHolder, sum);
+ }
+ break;
+ }
+ case FLOAT: {
+ float[] floatValues = blockValSet.getFloatValuesSV();
+ if (nullBitmap.getCardinality() < length) {
+ for (int i = 0; i < length; i++) {
+ if (!nullBitmap.contains(i)) {
+ if (Float.isFinite(floatValues[i])) {
+ sum = sum.add(BigDecimal.valueOf(floatValues[i]));
+ }
+ }
+ }
+ setAggregationResult(aggregationResultHolder, sum);
+ }
+ break;
+ }
+ case DOUBLE: {
+ double[] doubleValues = blockValSet.getDoubleValuesSV();
+ if (nullBitmap.getCardinality() < length) {
+ for (int i = 0; i < length; i++) {
+ if (!nullBitmap.contains(i)) {
+ // TODO(nhejazi): throw an exception here instead of ignoring
infinite values?
+ if (Double.isFinite(doubleValues[i])) {
Review Comment:
(minor) Suggest not having this check to keep the behavior consistent
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
