Jackie-Jiang commented on code in PR #9086:
URL: https://github.com/apache/pinot/pull/9086#discussion_r935024683
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -89,7 +122,33 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
@Override
public void aggregateGroupBySV(int length, int[] groupKeyArray,
GroupByResultHolder groupByResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
- if (blockValSetMap.isEmpty()) {
+ if (blockValSetMap.isEmpty() ||
!blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {
Review Comment:
Similar here, we should be able to simplify it into 3 conditions:
1. no value set
2. null handling enabled
3. star-tree
##########
pinot-core/src/test/java/org/apache/pinot/queries/BigDecimalQueriesTest.java:
##########
@@ -387,54 +387,6 @@ public void testQueries() {
assertEquals(row[0], BASE_BIG_DECIMAL.add(BigDecimal.valueOf(69)));
}
}
- {
Review Comment:
I don't get it. This test already exists without the null support on filter,
and should still work now right?
##########
pinot-core/src/test/java/org/apache/pinot/queries/BooleanNullEnabledQueriesTest.java:
##########
@@ -194,21 +194,6 @@ public void testQueries() {
}
}
}
- {
Review Comment:
Same here
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -611,7 +612,11 @@ private void addNewRow(int docId, GenericRow row) {
FieldSpec fieldSpec = indexContainer._fieldSpec;
DataType dataType = fieldSpec.getDataType();
- value =
indexContainer._valueAggregator.getInitialAggregatedValue(value);
+ if (indexContainer._isCountValueAggregator) {
Review Comment:
I see the reason. The reason is that we don't always replace the column
within `COUNT` with `*`. A better way to handle this is to change the
`CountValueAggregator` to implement `ValueAggregator<Object, Long>` so that it
can accept any object types.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java:
##########
@@ -75,6 +94,20 @@ public void aggregate(int length, AggregationResultHolder
aggregationResultHolde
Map<ExpressionContext, BlockValSet> blockValSetMap) {
if (blockValSetMap.isEmpty()) {
aggregationResultHolder.setValue(aggregationResultHolder.getDoubleResult() +
length);
+ } else if (!blockValSetMap.containsKey(STAR_TREE_COUNT_STAR_EXPRESSION)) {
+ if (!_nullHandlingEnabled) {
Review Comment:
Why is that? When null handling is enabled, we cannot use star-tree, so we
shouldn't need this check right? Basically when the `blockValSetMap` is empty,
either null handling is enabled or star-tree is used, and they should be mutual
exclusive
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumPrecisionAggregationFunction.java:
##########
@@ -48,8 +51,11 @@
public class SumPrecisionAggregationFunction extends
BaseSingleInputAggregationFunction<BigDecimal, BigDecimal> {
private final Integer _precision;
private final Integer _scale;
+ private final boolean _nullHandlingEnabled;
- public SumPrecisionAggregationFunction(List<ExpressionContext> arguments) {
+ private final Set<Integer> _groupKeysWithNonNullValue;
Review Comment:
We don't need this set. This method should be similar to the `AVG`
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/SumAggregationFunction.java:
##########
@@ -28,13 +30,23 @@
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 SumAggregationFunction extends
BaseSingleInputAggregationFunction<Double, Double> {
private static final double DEFAULT_VALUE = 0.0;
+ private final boolean _nullHandlingEnabled;
+
+ private final Set<Integer> _groupKeysWithNonNullValue;
Review Comment:
Let's fix this function similarly to the `MIN` and `MAX`: use object-based
result holder, and fix `extractAggregationResult()`
--
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]
