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: [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]
