egalpin commented on code in PR #10410:
URL: https://github.com/apache/pinot/pull/10410#discussion_r1134341506
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java:
##########
@@ -179,97 +178,91 @@ public static Object getConvertedFinalResult(DataTable
dataTable, ColumnDataType
}
/**
- * Build a filter operator from the given FilterContext.
- *
- * It returns the FilterPlanNode to allow reusing plan level components such
as predicate
- * evaluator map
+ * Build pairs of filtered aggregation functions and corresponding transform
operator.
*/
- public static Pair<FilterPlanNode, BaseFilterOperator>
buildFilterOperator(IndexSegment indexSegment,
- QueryContext queryContext, FilterContext filterContext) {
- FilterPlanNode filterPlanNode = new FilterPlanNode(indexSegment,
queryContext, filterContext);
- return Pair.of(filterPlanNode, filterPlanNode.run());
- }
+ public static List<Pair<AggregationFunction[], TransformOperator>>
buildFilteredAggregateTransformOperators(
Review Comment:
This method was already huge, so it's a not exactly a new "issue" introduced
by this PR, but what are your thoughts on breaking this method into a few
logical methods? Ex. the loop to create the hashmap of `Map<FilterContext,
Pair<BaseFilterOperator, List<AggregationFunction>>>` (line 200) could be its
own method? Likewise `// Create the transform operators` on line 230-256?
--
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]
