siddharthteotia commented on a change in pull request #7830:
URL: https://github.com/apache/pinot/pull/7830#discussion_r764484966



##########
File path: 
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java
##########
@@ -62,57 +68,31 @@ 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.getFilteredAggregationContexts()
+        .size() != 0;
 
-    FilterPlanNode filterPlanNode = new FilterPlanNode(_indexSegment, 
_queryContext);
-    BaseFilterOperator filterOperator = filterPlanNode.run();
+    Pair<Pair<BaseFilterOperator, TransformOperator>,
+        BaseOperator<IntermediateResultsBlock>> pair =
+        buildOperators(_queryContext.getFilter(), false);
 
-    // Use metadata/dictionary to solve the query if possible
-    // TODO: Use the same operator for both of them so that COUNT(*), MAX(col) 
can be optimized
-    if (filterOperator.isResultMatchingAll()) {
-      if (isFitForMetadataBasedPlan(aggregationFunctions)) {
-        return new MetadataBasedAggregationOperator(aggregationFunctions, 
_indexSegment.getSegmentMetadata(),
-            Collections.emptyMap());
-      } else if (isFitForDictionaryBasedPlan(aggregationFunctions, 
_indexSegment)) {
-        Map<String, Dictionary> dictionaryMap = new HashMap<>();
-        for (AggregationFunction aggregationFunction : aggregationFunctions) {
-          String column = ((ExpressionContext) 
aggregationFunction.getInputExpressions().get(0)).getIdentifier();
-          dictionaryMap.computeIfAbsent(column, k -> 
_indexSegment.getDataSource(k).getDictionary());
-        }
-        return new DictionaryBasedAggregationOperator(aggregationFunctions, 
dictionaryMap, numTotalDocs);
-      }
-    }
+    if (hasFilteredPredicates) {
+      int numTotalDocs = _indexSegment.getSegmentMetadata().getTotalDocs();
+      AggregationFunction[] aggregationFunctions = 
_queryContext.getAggregationFunctions();
 
-    // Use star-tree to solve the query if possible
-    List<StarTreeV2> starTrees = _indexSegment.getStarTrees();
-    if (starTrees != null && !StarTreeUtils.isStarTreeDisabled(_queryContext)) 
{
-      AggregationFunctionColumnPair[] aggregationFunctionColumnPairs =
-          StarTreeUtils.extractAggregationFunctionPairs(aggregationFunctions);
-      if (aggregationFunctionColumnPairs != null) {
-        Map<String, List<CompositePredicateEvaluator>> predicateEvaluatorsMap =
-            StarTreeUtils.extractPredicateEvaluatorsMap(_indexSegment, 
_queryContext.getFilter(),
-                filterPlanNode.getPredicateEvaluatorMap());
-        if (predicateEvaluatorsMap != null) {
-          for (StarTreeV2 starTreeV2 : starTrees) {
-            if (StarTreeUtils.isFitForStarTree(starTreeV2.getMetadata(), 
aggregationFunctionColumnPairs, null,
-                predicateEvaluatorsMap.keySet())) {
-              TransformOperator transformOperator =
-                  new StarTreeTransformPlanNode(starTreeV2, 
aggregationFunctionColumnPairs, null,
-                      predicateEvaluatorsMap, 
_queryContext.getDebugOptions()).run();
-              return new AggregationOperator(aggregationFunctions, 
transformOperator, numTotalDocs, true);
-            }
-          }
-        }
-      }
+      Set<ExpressionContext> expressionsToTransform =
+          
AggregationFunctionUtils.collectExpressionsToTransform(aggregationFunctions, 
null);
+      BaseOperator<IntermediateResultsBlock> aggregationOperator = 
pair.getRight();
+
+      assert pair != null && aggregationOperator != null;
+
+      //TODO: For non star tree, non filtered aggregation, share the main 
filter transform operator

Review comment:
       I suggest making this code a bit more readable. I find it super 
confusing because of all the Pair logic. Comments and descriptive variable 
names would be helpful




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

Reply via email to