[GitHub] [incubator-pinot] wuwenw commented on a change in pull request #7113: High Cardinality Order by Optimization

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org