Jackie-Jiang commented on a change in pull request #7916:
URL: https://github.com/apache/pinot/pull/7916#discussion_r784428201
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/ProjectionOperator.java
##########
@@ -63,7 +63,7 @@ protected ProjectionBlock getNextBlock() {
return null;
} else {
_dataBlockCache.initNewBlock(docIdSetBlock.getDocIdSet(),
docIdSetBlock.getSearchableLength());
- return new ProjectionBlock(_dataSourceMap, _dataBlockCache);
+ return new ProjectionBlock(_dataSourceMap, _dataBlockCache,
docIdSetBlock);
Review comment:
I don't think this change is required anymore. Same for the change in
`ProjectionBlock` and `TransformBlock`
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationGroupByOrderByPlanNode.java
##########
@@ -77,7 +77,7 @@ public AggregationGroupByOrderByOperator run() {
groupByExpressions, predicateEvaluatorsMap.keySet())) {
TransformOperator transformOperator =
new StarTreeTransformPlanNode(starTreeV2,
aggregationFunctionColumnPairs, groupByExpressions,
- predicateEvaluatorsMap,
_queryContext.getDebugOptions()).run();
+ predicateEvaluatorsMap, null,
_queryContext.getDebugOptions()).run();
Review comment:
This change should not be required anymore. We should only need to
change the `AggregationPlanNode`
##########
File path:
pinot-core/src/test/java/org/apache/pinot/core/query/request/context/utils/BrokerRequestToQueryContextConverterTest.java
##########
@@ -577,20 +575,6 @@ public void testHardcodedQueries() {
};
}
- @Test
- public void testFilteredAggregations() {
Review comment:
We should keep this test
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -90,9 +92,11 @@
// Pre-calculate the aggregation functions and columns for the query so that
it can be shared across all the segments
private AggregationFunction[] _aggregationFunctions;
- private List<Pair<AggregationFunction, FilterContext>>
_filteredAggregationFunctions;
+
+ private boolean _hasFilteredAggregations;
// TODO: Use Pair<FunctionContext, FilterContext> as key to support filtered
aggregations in order-by and post
// aggregation
+ private Map<Pair<FunctionContext, FilterContext>, Integer>
_filteredAgggregationsIndexMap;
Review comment:
```suggestion
private Map<Pair<FunctionContext, FilterContext>, Integer>
_filteredAggregationsIndexMap;
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/FilterBlock.java
##########
@@ -54,4 +65,4 @@ public BlockDocIdValueSet getBlockDocIdValueSet() {
public BlockMetadata getMetadata() {
throw new UnsupportedOperationException();
}
-}
+}
Review comment:
（minor) revert
##########
File path:
pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentAggregationMultiValueQueriesTest.java
##########
@@ -651,6 +651,13 @@ public void testNumGroupsLimit() {
assertTrue(brokerResponse.isNumGroupsLimitReached());
}
+ @Test
+ public void testFilteredAggregations() {
+ String query = "SELECT COUNT(*) FILTER(WHERE column1 > 5) FROM testTable
WHERE column3 > 0";
+ BrokerResponseNative brokerResponse = getBrokerResponseForSqlQuery(query);
+ assertEquals(brokerResponse.getResultTable().getRows().size(), 1);
Review comment:
Also verify the result?
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -90,9 +92,11 @@
// Pre-calculate the aggregation functions and columns for the query so that
it can be shared across all the segments
private AggregationFunction[] _aggregationFunctions;
- private List<Pair<AggregationFunction, FilterContext>>
_filteredAggregationFunctions;
+
+ private boolean _hasFilteredAggregations;
// TODO: Use Pair<FunctionContext, FilterContext> as key to support filtered
aggregations in order-by and post
Review comment:
This TODO can be removed
##########
File path:
pinot-core/src/test/java/org/apache/pinot/queries/InnerSegmentAggregationSingleValueQueriesTest.java
##########
@@ -49,27 +49,49 @@
" GROUP BY column1, column3, column6, column7, column9, column11,
column12, column17, column18";
@Test
- public void testAggregationOnly() {
Review comment:
Don't remove the existing `testAggregationOnly()`. Since we added a new
`FilteredAggregationsTest`, not sure if we still want to add this test. If so,
let's keep them as 2 separate tests
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
##########
@@ -62,57 +69,25 @@ public AggregationPlanNode(IndexSegment indexSegment,
QueryContext queryContext)
public Operator<IntermediateResultsBlock> run() {
assert _queryContext.getAggregationFunctions() != null;
- int numTotalDocs = _indexSegment.getSegmentMetadata().getTotalDocs();
- AggregationFunction[] aggregationFunctions =
_queryContext.getAggregationFunctions();
+ boolean hasFilteredPredicates = _queryContext.isHasFilteredAggregations();
- FilterPlanNode filterPlanNode = new FilterPlanNode(_indexSegment,
_queryContext);
- BaseFilterOperator filterOperator = filterPlanNode.run();
+ Pair<FilterPlanNode, BaseFilterOperator> filterOperatorPair =
Review comment:
Suggest splitting the logic of filtered aggregation and regular
aggregation. Currently the logic is mixed in `buildOperators()` which is hard
to read or debug.
We can keep the regular aggregation handling unchanged, and add the code
path for the filtered aggregations. The metadata/dictionary/startree based
operator does not apply to the filtered aggregation.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -90,9 +92,11 @@
// Pre-calculate the aggregation functions and columns for the query so that
it can be shared across all the segments
private AggregationFunction[] _aggregationFunctions;
- private List<Pair<AggregationFunction, FilterContext>>
_filteredAggregationFunctions;
+
Review comment:
Can we keep this list and remove the `FilterableAggregationFunction`? We
should avoid adding this special aggregation function if possible because the
filter is not part of the aggregation
--
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
