Jackie-Jiang commented on a change in pull request #6991:
URL: https://github.com/apache/incubator-pinot/pull/6991#discussion_r645876332
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java
##########
@@ -106,8 +116,14 @@ protected IntermediateResultsBlock getNextBlock() {
groupByExecutor.process(transformBlock);
}
- // Build intermediate result block based on aggregation group-by result
from the executor
- return new IntermediateResultsBlock(_aggregationFunctions,
groupByExecutor.getResult(), _dataSchema);
+ // Trim is off or no need to trim
+ if (_trimSize <= 0 || groupByExecutor.getNumGroups() <= _trimSize) {
+ // Build intermediate result block based on aggregation group-by result
from the executor
+ return new IntermediateResultsBlock(_aggregationFunctions,
groupByExecutor.getResult(), _dataSchema);
+ }
+ TableResizer tableResizer = new TableResizer(_dataSchema, _queryContext);
+ Collection<IntermediateRecord> intermediate =
groupByExecutor.trimGroupByResult(_trimSize, tableResizer);
Review comment:
(nit)
```suggestion
Collection<IntermediateRecord> intermediateRecords =
groupByExecutor.trimGroupByResult(_trimSize, tableResizer);
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationGroupByOrderByPlanNode.java
##########
@@ -58,6 +63,14 @@ public AggregationGroupByOrderByPlanNode(IndexSegment
indexSegment, QueryContext
List<ExpressionContext> groupByExpressions =
queryContext.getGroupByExpressions();
assert groupByExpressions != null;
_groupByExpressions = groupByExpressions.toArray(new ExpressionContext[0]);
+ _queryContext = queryContext;
+
+ // Only trim if there is OrderBy and has a positive trim size
+ if (queryContext.getOrderByExpressions() != null && minSegmentTrimSize >
0) {
Review comment:
I feel we might want to pass `minSegmentTrimSize` to the operator, and
the operator can decide whether to trim it or not
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java
##########
@@ -38,28 +42,34 @@
@SuppressWarnings("rawtypes")
public class AggregationGroupByOrderByOperator extends
BaseOperator<IntermediateResultsBlock> {
private static final String OPERATOR_NAME =
"AggregationGroupByOrderByOperator";
+ private static final int TRIM_OFF = -1;
Review comment:
Remove
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -187,6 +216,7 @@ public PlanNode makeStreamingSegmentPlanNode(IndexSegment
indexSegment, QueryCon
}
}
+
Review comment:
(nit) remove
--
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.
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
