You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/06/22 13:06:09 UTC
[GitHub] [doris] EmmyMiao87 commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
EmmyMiao87 commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r903705675
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalAggregation.java:
##########
@@ -48,31 +49,31 @@ public class PhysicalAggregation extends PhysicalUnaryOperator {
*
* @param groupByExprList group by expr list.
* @param aggExprList agg expr list.
- * @param partitionExprList partition expr list, used for analytic agg.
+ * @param partitionExprList partition expr list, used for analytic agg.
* @param usingStream whether it's stream agg.
*/
- public PhysicalAggregation(List<Expression> groupByExprList, List<Expression> aggExprList,
+ public PhysicalAggregation(List<Expression> groupByExprList, List<? extends NamedExpression> aggExprList,
List<Expression> partitionExprList, AggPhase aggPhase, boolean usingStream) {
super(OperatorType.PHYSICAL_AGGREGATION);
this.groupByExprList = groupByExprList;
this.aggExprList = aggExprList;
- this.partitionExprList = partitionExprList;
this.aggPhase = aggPhase;
+ this.partitionExprList = partitionExprList;
this.usingStream = usingStream;
}
+ public AggPhase getAggPhase() {
+ return aggPhase;
+ }
+
public List<Expression> getGroupByExprList() {
return groupByExprList;
}
- public List<Expression> getAggExprList() {
+ public List<? extends NamedExpression> getAggExprList() {
Review Comment:
Why is the data type here not `NamedExpression`?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalSort.java:
##########
@@ -31,84 +32,51 @@
/**
* Logical Sort plan operator.
- *
+ * <p>
* eg: select * from table order by a, b desc;
* sortItems: list of column information after order by. eg:[a, asc],[b, desc].
* SortItems: Contains order expression information and sorting method. Default is ascending.
*/
public class LogicalSort extends LogicalUnaryOperator {
- private List<SortItems> sortItems;
+ // Default offset is 0.
+ private int offset = 0;
+
+ private final List<OrderKey> sortItems;
Review Comment:
Please unify the naming in the code. Don't `sortitem` for a while, `order key` for a while
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalSort.java:
##########
@@ -31,84 +32,51 @@
/**
* Logical Sort plan operator.
- *
+ * <p>
* eg: select * from table order by a, b desc;
* sortItems: list of column information after order by. eg:[a, asc],[b, desc].
* SortItems: Contains order expression information and sorting method. Default is ascending.
*/
public class LogicalSort extends LogicalUnaryOperator {
- private List<SortItems> sortItems;
+ // Default offset is 0.
+ private int offset = 0;
+
+ private final List<OrderKey> sortItems;
/**
* Constructor for SortItems.
*/
- public LogicalSort(List<SortItems> sortItems) {
+ public LogicalSort(List<OrderKey> sortItems) {
super(OperatorType.LOGICAL_SORT);
this.sortItems = Objects.requireNonNull(sortItems, "sorItems can not be null");
}
- @Override
- public String toString() {
- return "Sort (" + StringUtils.join(sortItems, ", ") + ")";
- }
-
@Override
public List<Slot> computeOutput(Plan input) {
return input.getOutput();
}
- /**
- * Get SortItems.
- *
- * @return List of SortItems.
- */
- public List<SortItems> getSortItems() {
+ public List<OrderKey> getSortItems() {
Review Comment:
same as above
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalSort.java:
##########
@@ -33,22 +33,19 @@
* Physical sort plan operator.
*/
public class PhysicalSort extends PhysicalUnaryOperator {
-
+ // Default offset is 0.
private final int offset;
- private final int limit;
-
private final List<OrderKey> orderList;
-
+ // TODO(wj): Do we need it? If offset is 0 and limit != 0, we can think it's topN.
Review Comment:
Doris' topn and sort are actually completely different logics at the execution layer. I'm thinking whether it is necessary to distinguish these two operators in terms of physical operators?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalSort.java:
##########
@@ -33,22 +33,19 @@
* Physical sort plan operator.
*/
public class PhysicalSort extends PhysicalUnaryOperator {
-
+ // Default offset is 0.
private final int offset;
- private final int limit;
-
private final List<OrderKey> orderList;
-
+ // TODO(wj): Do we need it? If offset is 0 and limit != 0, we can think it's topN.
private final boolean useTopN;
/**
* Constructor of PhysicalHashJoinNode.
*/
- public PhysicalSort(int offset, int limit, List<OrderKey> orderList, boolean useTopN) {
- super(OperatorType.PHYSICAL_SORT);
+ public PhysicalSort(List<OrderKey> orderList, long limit, int offset, boolean useTopN) {
Review Comment:
For example, we may have a physical operator of heap sort, or a physical operator of merge sort.
--
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