[GitHub] [doris] morrySnow commented on a diff in pull request #16797: [feature](Nereids): add rule split limit into two phase

["morrySnow (via GitHub)" <gi...@apache.org>]

morrySnow commented on code in PR #16797:
URL: https://github.com/apache/doris/pull/16797#discussion_r1108242007


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -1338,38 +1338,15 @@ public PlanFragment visitPhysicalLimit(PhysicalLimit<? extends Plan> physicalLim
         if (inputFragment == null) {
             return inputFragment;
         }
-
+        // For case globalLimit(l, o) -> LocalLimit(l+o, 0), that is the LocalLimit has already gathered
+        // The globalLimit can overwrite the limit and offset, so it's still correct
         PlanNode child = inputFragment.getPlanRoot();
-
-        // physical plan:  limit --> sort
-        // after translate, it could be:
-        // 1. limit->sort => set (limit and offset) on sort
-        // 2. limit->exchange->sort => set (limit and offset) on exchange, set sort.limit = limit+offset
-        if (child instanceof SortNode) {
-            SortNode sort = (SortNode) child;
-            sort.setLimit(physicalLimit.getLimit());
-            sort.setOffset(physicalLimit.getOffset());
-            return inputFragment;
-        }
-        if (child instanceof ExchangeNode) {
-            ExchangeNode exchangeNode = (ExchangeNode) child;
-            exchangeNode.setLimit(physicalLimit.getLimit());
-            // we do not check if this is a merging exchange here,
-            // since this guaranteed by translating logic plan to physical plan
-            exchangeNode.setOffset(physicalLimit.getOffset());
-            if (exchangeNode.getChild(0) instanceof SortNode) {
-                SortNode sort = (SortNode) exchangeNode.getChild(0);
-                sort.setLimit(physicalLimit.getLimit() + physicalLimit.getOffset());
-                sort.setOffset(0);
-            }
-            return inputFragment;
-        }
-        // for other PlanNode, just set limit as limit+offset
-        child.setLimit(physicalLimit.getLimit() + physicalLimit.getOffset());
-        PlanFragment planFragment = exchangeToMergeFragment(inputFragment, context);
-        planFragment.getPlanRoot().setLimit(physicalLimit.getLimit());
-        planFragment.getPlanRoot().setOffSetDirectly(physicalLimit.getOffset());
-        return planFragment;
+        child.setLimit(physicalLimit.getLimit());
+        child.setOffset(physicalLimit.getOffset());
+        // TODO: add a rule in cascades
+        //      before: GlobalLimit -> LocalLimit -> GlobalSort -> Gather -> LocalSort
+        //      after: GlobalLimit -> GlobalSort -> Gather ->LocalLimit -> LocalSort
+        return inputFragment;

Review Comment:
   if we have not rule to rewrite Limit(Sort) to TopN, and do not set limit and offset on ExchangeNode's child, the performance of `order by x limit n` is not acceptable.



##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalLimit.java:
##########
@@ -44,19 +45,28 @@
  * offset 100
  */
 public class LogicalLimit<CHILD_TYPE extends Plan> extends LogicalUnary<CHILD_TYPE> implements Limit {
-
+    private final LimitPhase phase;
     private final long limit;
     private final long offset;
 
-    public LogicalLimit(long limit, long offset, CHILD_TYPE child) {
-        this(limit, offset, Optional.empty(), Optional.empty(), child);
+    public LogicalLimit(long limit, long offset, CHILD_TYPE child, LimitPhase phase) {
+        this(limit, offset, Optional.empty(), Optional.empty(), child, phase);

Review Comment:
   children should be the last parameter



-- 
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@doris.apache.org

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


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