wuwenw commented on a change in pull request #7113:
URL: https://github.com/apache/incubator-pinot/pull/7113#discussion_r668111237
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationGroupByOrderByPlanNode.java
##########
@@ -97,14 +116,38 @@ public AggregationGroupByOrderByOperator run() {
int numTotalDocs = _indexSegment.getSegmentMetadata().getTotalDocs();
if (_transformPlanNode != null) {
// Do not use star-tree
- return new AggregationGroupByOrderByOperator(_aggregationFunctions,
_groupByExpressions,
- _maxInitialResultHolderCapacity, _numGroupsLimit,
_minSegmentTrimSize, _transformPlanNode.run(), numTotalDocs,
- _queryContext, false);
+ return new AggregationGroupByOrderByOperator(_indexSegment,
_aggregationFunctions, _groupByExpressions,
+ _orderByExpressionContexts.toArray(new OrderByExpressionContext[0]),
_maxInitialResultHolderCapacity,
+ _numGroupsLimit, _minSegmentTrimSize, _transformPlanNode.run(),
numTotalDocs, _queryContext,
+ _enableGroupByOpt, false);
} else {
// Use star-tree
- return new AggregationGroupByOrderByOperator(_aggregationFunctions,
_groupByExpressions,
- _maxInitialResultHolderCapacity, _numGroupsLimit,
_minSegmentTrimSize, _starTreeTransformPlanNode.run(),
- numTotalDocs, _queryContext, true);
+ return new AggregationGroupByOrderByOperator(_indexSegment,
_aggregationFunctions, _groupByExpressions,
+ _orderByExpressionContexts.toArray(new OrderByExpressionContext[0]),
_maxInitialResultHolderCapacity,
+ _numGroupsLimit, _minSegmentTrimSize,
_starTreeTransformPlanNode.run(), numTotalDocs, _queryContext,
+ _enableGroupByOpt, true);
+ }
+ }
+
+ private boolean checkOrderByOptimization() {
+ if (_queryContext.getHavingFilter() != null) {
+ return false;
+ }
+ Set<ExpressionContext> orderByExpressionsSet = new HashSet<>();
+ // Filter out function expressions
+ for (OrderByExpressionContext orderByExpressionContext :
_orderByExpressionContexts) {
+ ExpressionContext expression = orderByExpressionContext.getExpression();
+ if (expression.getType() == ExpressionContext.Type.FUNCTION) {
+ return false;
+ }
+ orderByExpressionsSet.add(expression);
+ }
+ // Add group by expressions to order by expressions
+ for (ExpressionContext groupByExpression: _groupByExpressions) {
+ if (!orderByExpressionsSet.contains(groupByExpression)) {
+ _orderByExpressionContexts.add(new
OrderByExpressionContext(groupByExpression, true));
Review comment:
fixed
> We should not directly modify the query context because it is shared among
multiple segments, and can cause race condition. Ideally this rewrite should be
done either on broker side or before the query planning on server side.
> For now, we can keep a local copy of the order by expressions list.
--
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]
