[GitHub] [doris] morrySnow commented on a diff in pull request #16586: [refactor](Nereids): add two phase sort

[via GitHub]

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


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalTopNToPhysicalTopN.java:
##########
@@ -19,20 +19,33 @@
 
 import org.apache.doris.nereids.rules.Rule;
 import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.SortPhase;
+import org.apache.doris.nereids.trees.plans.logical.LogicalTopN;
 import org.apache.doris.nereids.trees.plans.physical.PhysicalTopN;
 
+import com.google.common.collect.Lists;
+
+import java.util.List;
+
 /**
  * Implementation rule that convert logical top-n to physical top-n.
  */
 public class LogicalTopNToPhysicalTopN extends OneImplementationRuleFactory {
     @Override
     public Rule build() {
-        return logicalTopN().then(topN -> new PhysicalTopN<>(
-                topN.getOrderKeys(),
-                topN.getLimit(),
-                topN.getOffset(),
-                topN.getLogicalProperties(),
-                topN.child())
-        ).toRule(RuleType.LOGICAL_TOP_N_TO_PHYSICAL_TOP_N_RULE);
+        return logicalTopN().thenApplyMulti(ctx -> twoPhaseSort(ctx.root))
+                .toRule(RuleType.LOGICAL_TOP_N_TO_PHYSICAL_TOP_N_RULE);
+    }
+
+    private List<PhysicalTopN<? extends Plan>> twoPhaseSort(LogicalTopN 
logicalTopN) {
+        PhysicalTopN localSort = new PhysicalTopN(logicalTopN.getOrderKeys(), 
logicalTopN.getLimit(),
+                logicalTopN.getOffset(), logicalTopN.getLogicalProperties(), 
logicalTopN.child(0),
+                SortPhase.LOCAL_SORT);
+        PhysicalTopN twoPhaseSort = new 
PhysicalTopN<>(logicalTopN.getOrderKeys(), logicalTopN.getLimit(),
+                logicalTopN.getOffset(), logicalTopN.getLogicalProperties(), 
localSort, SortPhase.MERGE_SORT);
+        PhysicalTopN onePhaseSort = new 
PhysicalTopN<>(logicalTopN.getOrderKeys(), logicalTopN.getLimit(),
+                logicalTopN.getOffset(), logicalTopN.getLogicalProperties(), 
localSort.child(0), SortPhase.GATHER_SORT);
+        return Lists.newArrayList(twoPhaseSort, onePhaseSort);

Review Comment:
   we should only return two sort plan:
   - gather sort: require child's distribution is GATHER
   - merge sort: first stage require child's distribution is ANY, and second 
stage require is GATHER
   
   then, cost and enforce could generate right plan for the two type of sort.
   i think the main reason u use three types is easy for translation, right?
   so, we could have two type of sort.
   - LOCAL: translate to sort node
   - MERGE: merge sort info into its child exchange node.
   
   and, u could generate enforce require as AggregateStrategy do.



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

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


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