Jackie-Jiang commented on code in PR #9086:
URL: https://github.com/apache/pinot/pull/9086#discussion_r933876211
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/RequestContextUtils.java:
##########
@@ -95,6 +90,9 @@ public static FunctionContext getFunction(Function
thriftFunction) {
for (Expression operand : operands) {
arguments.add(getExpression(operand));
}
+ if (arguments.size() == 0 &&
functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
Review Comment:
Let's add a TODO stating this is a work-around for multi-stage query engine
which might pass COUNT without argument, and should be removed once that is
fixed.
(nit)
```suggestion
if (arguments.isEmpty() &&
functionName.equalsIgnoreCase(AggregationFunctionType.COUNT.getName())) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/FastFilteredCountOperator.java:
##########
@@ -38,14 +38,16 @@ public class FastFilteredCountOperator extends
BaseOperator<IntermediateResultsB
private final BaseFilterOperator _filterOperator;
private final AggregationFunction[] _aggregationFunctions;
private final SegmentMetadata _segmentMetadata;
+ private final boolean _nullHandlingEnabled;
private long _docsCounted;
public FastFilteredCountOperator(AggregationFunction[] aggregationFunctions,
BaseFilterOperator filterOperator,
- SegmentMetadata segmentMetadata) {
+ SegmentMetadata segmentMetadata, boolean nullHandlingEnabled) {
Review Comment:
This operator cannot support null handling. We should not use it when null
handling is enabled
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/MaxAggregationFunction.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 MaxAggregationFunction extends
BaseSingleInputAggregationFunction<Double, Double> {
private static final double DEFAULT_INITIAL_VALUE = Double.NEGATIVE_INFINITY;
+ private final boolean _nullHandlingEnabled;
+
+ private final Set<Integer> _groupKeysWithNonNullValue;
Review Comment:
This requires extra lookup.
Instead, we can use object based result holder when null handling is
enabled. We need it anyway in order to extract the aggregation result (no
group-by) which is not properly handled right now. Same for `MIN` and `SUM`
##########
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:
Could this branch be hit?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunction.java:
##########
@@ -157,6 +239,9 @@ public AvgPair
extractAggregationResult(AggregationResultHolder aggregationResul
@Override
public AvgPair extractGroupByResult(GroupByResultHolder groupByResultHolder,
int groupKey) {
Review Comment:
`extractAggregationResult()` also needs to be modified to return `null` when
null handling is enabled
##########
pinot-perf/src/main/java/org/apache/pinot/perf/BenchmarkScanDocIdIterators.java:
##########
@@ -123,7 +123,8 @@ public void tearDown()
@Benchmark
public MutableRoaringBitmap benchmarkSVLong() {
- return new SVScanDocIdIterator(_predicateEvaluator, _readerV2,
_numDocs).applyAnd(_bitmap);
+ return new SVScanDocIdIterator(_predicateEvaluator, _readerV2, _numDocs)
Review Comment:
(minor) revert
##########
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:
Why do we remove these 2 tests?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunction.java:
##########
@@ -30,13 +32,23 @@
import org.apache.pinot.segment.local.customobject.AvgPair;
import org.apache.pinot.segment.spi.AggregationFunctionType;
import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.roaringbitmap.RoaringBitmap;
public class AvgAggregationFunction extends
BaseSingleInputAggregationFunction<AvgPair, Double> {
private static final double DEFAULT_FINAL_RESULT = Double.NEGATIVE_INFINITY;
+ private final boolean _nullHandlingEnabled;
+
+ private final Set<Integer> _groupKeysWithNonNullValue;
Review Comment:
This shouldn't be needed since the result holder can already track null
values
##########
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:
Is this related?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/selection/SelectionOperatorUtils.java:
##########
@@ -94,27 +94,26 @@ public static List<ExpressionContext>
extractExpressions(QueryContext queryConte
}
List<ExpressionContext> selectExpressions =
queryContext.getSelectExpressions();
- if (selectExpressions.size() == 1 &&
selectExpressions.get(0).equals(IDENTIFIER_STAR)) {
- // For 'SELECT *', sort all columns (ignore columns that start with '$')
so that the order is deterministic
- Set<String> allColumns = indexSegment.getColumnNames();
- List<String> selectColumns = new ArrayList<>(allColumns.size());
- for (String column : allColumns) {
- if (column.charAt(0) != '$') {
- selectColumns.add(column);
- }
- }
- selectColumns.sort(null);
- for (String column : selectColumns) {
- ExpressionContext expression = ExpressionContext.forIdentifier(column);
- if (!expressionSet.contains(expression)) {
- expressions.add(expression);
+ for (ExpressionContext selectExpression : selectExpressions) {
Review Comment:
This part seems unrelated to this PR. This rewrite should already be
performed on the broker side, so shouldn't be needed
##########
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:
Suggest changing the branching logic as following for clarity:
```
if (blockValSetMap.isEmpty()) {
...
} else if (_nullHandlingEnabled) {
...
} else {
// Star-tree pre-aggregated values
...
}
##########
pinot-core/src/test/java/org/apache/pinot/queries/BooleanNullEnabledQueriesTest.java:
##########
@@ -194,21 +194,6 @@ public void testQueries() {
}
}
}
- {
Review Comment:
Why removing this test?
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -212,7 +214,8 @@ public Operator<IntermediateResultsBlock>
buildNonFilteredAggOperator() {
TransformOperator transformOperator =
new StarTreeTransformPlanNode(_queryContext, starTreeV2,
aggregationFunctionColumnPairs, null,
predicateEvaluatorsMap).run();
- return new AggregationOperator(aggregationFunctions,
transformOperator, numTotalDocs, true);
+ return new AggregationOperator(aggregationFunctions,
transformOperator, numTotalDocs, true,
Review Comment:
We should also skip star-tree when null handling is enabled
--
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
