morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r906029487
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalHeapSort.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 {
+public class LogicalHeapSort extends LogicalUnaryOperator {
- private List<SortItems> sortItems;
+ // Default offset is 0.
+ private int offset = 0;
+
+ private final List<OrderKey> orderKeys;
/**
* Constructor for SortItems.
*/
- public LogicalSort(List<SortItems> sortItems) {
+ public LogicalHeapSort(List<OrderKey> orderKeys) {
super(OperatorType.LOGICAL_SORT);
- this.sortItems = Objects.requireNonNull(sortItems, "sorItems can not
be null");
- }
-
- @Override
- public String toString() {
- return "Sort (" + StringUtils.join(sortItems, ", ") + ")";
+ this.orderKeys = Objects.requireNonNull(orderKeys, "sorItems can not
be null");
Review Comment:
```suggestion
this.orderKeys = Objects.requireNonNull(orderKeys, "orderKeys can
not be null");
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/OperatorVisitor.java:
##########
@@ -44,7 +44,7 @@ public R visitPhysicalOlapScan(PhysicalOlapScan
physicalOlapScan, C context) {
return null;
}
- public R visitPhysicalSort(PhysicalSort physicalSort, C context) {
+ public R visitPhysicalSort(PhysicalHeapSort physicalHeapSort, C context) {
Review Comment:
```suggestion
public R visitPhysicalHeapSort(PhysicalHeapSort physicalHeapSort, C
context) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalAggregation.java:
##########
@@ -31,68 +32,70 @@
/**
* Logical Aggregation plan operator.
- *
- *eg:select a, sum(b), c from table group by a, c;
+ * <p>
+ * eg:select a, sum(b), c from table group by a, c;
* groupByExpressions: Column field after group by. eg: a, c;
* outputExpressions: Column field after select. eg: a, sum(b), c;
- *
+ * <p>
* Each agg node only contains the select statement field of the same layer,
* and other agg nodes in the subquery contain.
*/
public class LogicalAggregation extends LogicalUnaryOperator {
- private final List<Expression> groupByExpressions;
- private final List<? extends NamedExpression> outputExpressions;
+ private final List<Expression> groupByExprList;
+ private final List<NamedExpression> aggExprList;
+ private List<Expression> partitionExprList;
+
+ private AggPhase aggPhase;
/**
* Desc: Constructor for LogicalAggregation.
*/
- public LogicalAggregation(List<Expression> groupByExpressions,
- List<? extends NamedExpression> outputExpressions) {
+ public LogicalAggregation(List<Expression> groupByExprList,
List<NamedExpression> aggExprList) {
super(OperatorType.LOGICAL_AGGREGATION);
- this.groupByExpressions = groupByExpressions;
- this.outputExpressions = outputExpressions;
+ this.groupByExprList = groupByExprList;
+ this.aggExprList = aggExprList;
Review Comment:
```suggestion
this. outputExpressionList = outputExpressionList;
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalHeapSort.java:
##########
@@ -32,23 +32,20 @@
/**
* Physical sort plan operator.
*/
-public class PhysicalSort extends PhysicalUnaryOperator {
-
+public class PhysicalHeapSort 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 PhysicalHeapSort(List<OrderKey> orderList, long limit, int offset,
boolean useTopN) {
Review Comment:
```suggestion
public PhysicalHeapSort(List<OrderKey> orderKeys, long limit, int
offset, boolean useTopN) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -210,30 +204,20 @@ public List<Expression>
visitNamedExpressionSeq(NamedExpressionSeqContext ctx) {
*
* <p>Note that query hints are ignored (both by the parser and the
builder).
*/
- private LogicalPlan withSelectQuerySpecification(
- ParserRuleContext ctx,
- SelectClauseContext selectClause,
- WhereClauseContext whereClause,
- LogicalPlan relation,
- AggClauseContext aggClause) {
+ private LogicalPlan withSelectQuerySpecification(ParserRuleContext ctx,
SelectClauseContext selectClause,
+ WhereClauseContext whereClause, LogicalPlan relation,
AggClauseContext aggClause) {
Supplier<LogicalPlan> f = () -> {
// Filter(expression(ctx.booleanExpression), plan);
- LogicalPlan plan = visitCommonSelectQueryClausePlan(
- relation,
- visitNamedExpressionSeq(selectClause.namedExpressionSeq()),
- whereClause,
- aggClause);
+ LogicalPlan plan = visitCommonSelectQueryClausePlan(relation,
+
visitNamedExpressionSeq(selectClause.namedExpressionSeq()), whereClause,
aggClause);
// TODO: process hint
return plan;
};
return ParserUtils.withOrigin(ctx, f);
}
- private LogicalPlan visitCommonSelectQueryClausePlan(
- LogicalPlan relation,
- List<Expression> expressions,
- WhereClauseContext whereClause,
- AggClauseContext aggClause) {
Review Comment:
the original style is easy to read
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -337,15 +318,12 @@ private LogicalPlan withAggClause(List<NamedExpression>
aggExpressions,
* @param ctx SortItemContext
* @return SortItems
*/
- public SortItems genSortItems(SortItemContext ctx) {
- OrderDirection orderDirection;
Review Comment:
enum maybe better to read
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalHeapSort.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 {
+public class LogicalHeapSort extends LogicalUnaryOperator {
Review Comment:
sort in Logical should not distinguish physical implementation. So class
name should be LogicalSort
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -210,30 +204,20 @@ public List<Expression>
visitNamedExpressionSeq(NamedExpressionSeqContext ctx) {
*
* <p>Note that query hints are ignored (both by the parser and the
builder).
*/
- private LogicalPlan withSelectQuerySpecification(
- ParserRuleContext ctx,
- SelectClauseContext selectClause,
- WhereClauseContext whereClause,
- LogicalPlan relation,
- AggClauseContext aggClause) {
+ private LogicalPlan withSelectQuerySpecification(ParserRuleContext ctx,
SelectClauseContext selectClause,
+ WhereClauseContext whereClause, LogicalPlan relation,
AggClauseContext aggClause) {
Review Comment:
the original style is easy to read
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalAggToHashAgg.java:
##########
@@ -0,0 +1,42 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules.implementation;
+
+import org.apache.doris.nereids.operators.plans.physical.PhysicalAggregation;
+import org.apache.doris.nereids.rules.Rule;
+import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.trees.plans.Plan;
+
+/**
+ * Implementation rule that convert logical aggregation to physical hash
aggregation.
+ */
+public class LogicalAggToHashAgg extends OneImplementationRuleFactory {
+ @Override
+ public Rule<Plan> build() {
+ return logicalAggregation().then(agg -> plan(
+ new PhysicalAggregation(
+ agg.getOperator().getGroupByExprList(),
+ agg.getOperator().getAggExprList(),
+ agg.getOperator().getPartitionExprList(),
+ agg.getOperator().getAggPhase(),
+ false),
Review Comment:
add a todo for use a function to judge whether use stream
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindRelation.java:
##########
@@ -45,13 +45,13 @@ public Rule<Plan> build() {
case 1: {
List<String> qualifier =
Lists.newArrayList(connectContext.getDatabase(), nameParts.get(0));
Table table = getTable(qualifier,
connectContext.getCatalog());
- LogicalRelation relation = new LogicalRelation(table,
qualifier);
- return new LogicalLeafPlan<>(relation);
+ LogicalOlapScan olapScan = new LogicalOlapScan(table,
qualifier);
Review Comment:
should generate different Scan sub class according to table's type
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalHeapSort.java:
##########
@@ -32,23 +32,20 @@
/**
* Physical sort plan operator.
*/
-public class PhysicalSort extends PhysicalUnaryOperator {
-
+public class PhysicalHeapSort 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;
Review Comment:
remove that since TopN and HeapSort is same thing
--
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
