Jackie-Jiang commented on code in PR #10410:
URL: https://github.com/apache/pinot/pull/10410#discussion_r1134470106
##########
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 is very specific for the filtered aggregate, which groups the
aggregates with the same filter and build a transform operator for them.
IMO it is more readable to have all the logic in the same method. If we
extract the creation of maps in separate methods, each method will have some
assumptions such as main filter not empty, comparing combined filter with main
filter etc. which makes them very hard to be reused. In the future if we find
part of this method can be reused by another method, we can extract it out at
that time.
--
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]
