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 08:28:36 UTC
[GitHub] [doris] jackwener opened a new pull request, #10335: [enhancement](Nereids): add more implmentation rules.
jackwener opened a new pull request, #10335:
URL: https://github.com/apache/doris/pull/10335
# Proposed changes
Issue Number: close #xxx
## Problem Summary:
Add more implmentation rules.
Current some `logical` and `physical` operator is different. I change some code to make them match.
## Checklist(Required)
1. Does it affect the original behavior: (No)
2. Has unit tests been added: (No Need)
3. Has document been added or modified: (No Need)
4. Does it need to update dependencies: (No)
5. Are there any changes that cannot be rolled back: (No)
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r907277808
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/RuleType.java:
##########
@@ -42,12 +42,12 @@ public enum RuleType {
LOGICAL_JOIN_EXCHANGE(RuleTypeClass.EXPLORATION),
// implementation rules
+ LOGICAL_AGG_TO_HASH_AGG_RULE(RuleTypeClass.IMPLEMENTATION),
LOGICAL_JOIN_TO_HASH_JOIN_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_PROJECTION_TO_PHYSICAL_PROJECTION_RULE(RuleTypeClass.IMPLEMENTATION),
+ LOGICAL_PROJECT_TO_PHYSICAL_PROJECT_RULE(RuleTypeClass.IMPLEMENTATION),
LOGICAL_FILTER_TO_PHYSICAL_FILTER_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_LIMIT_TO_PHYSICAL_LIMIT_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_JOIN_TO_HASH_AGG_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_JOIN_TO_OLAP_SCAN_RULE(RuleTypeClass.IMPLEMENTATION),
+ LOGICAL_HEAP_SORT_TO_PHYSICAL_HEAP_SORT_RULE(RuleTypeClass.IMPLEMENTATION),
Review Comment:
we do not add the 'Physical' prefix if we have more than one physical implementation for one operator
--
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
[GitHub] [doris] jackwener commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
jackwener commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r903750067
##########
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:
It's is a question worth considering.
--
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
[GitHub] [doris] yinzhijian commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
yinzhijian commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r907155423
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalOlapScanToPhysicalOlapScan.java:
##########
@@ -0,0 +1,37 @@
+// 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.PhysicalHashJoin;
+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 OlapScan to physical OlapScan.
+ */
+public class LogicalOlapScanToPhysicalOlapScan extends OneImplementationRuleFactory {
+ @Override
+ public Rule<Plan> build() {
+ return logicalJoin().then(join -> plan(
+ new PhysicalHashJoin(join.operator.getJoinType(), join.operator.getCondition().get()),
Review Comment:
PhysicalHashJoin?
--
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
[GitHub] [doris] EmmyMiao87 merged pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 merged PR #10335:
URL: https://github.com/apache/doris/pull/10335
--
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
[GitHub] [doris] morrySnow commented on pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on PR #10335:
URL: https://github.com/apache/doris/pull/10335#issuecomment-1162859447
Could you list what implementation rules add in this PR in '**Proposed changes**'
--
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
[GitHub] [doris] EmmyMiao87 commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r907119613
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -180,7 +179,7 @@ public LogicalPlan visitQuery(QueryContext ctx) {
}
private LogicalPlan withQueryOrganization(QueryOrganizationContext ctx, LogicalPlan children) {
- List<SortItems> sortItems = visitQueryOrganization(ctx);
+ List<OrderKey> sortItems = visitQueryOrganization(ctx);
Review Comment:
```suggestion
List<OrderKey> orderKeys = visitQueryOrganization(ctx);
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -368,8 +353,8 @@ public SortItems genSortItems(SortItemContext ctx) {
* @param ctx QueryOrganizationContext
* @return List of SortItems
*/
- public List<SortItems> visitQueryOrganization(QueryOrganizationContext ctx) {
- List<SortItems> sortItems = new ArrayList<>();
+ public List<OrderKey> visitQueryOrganization(QueryOrganizationContext ctx) {
+ List<OrderKey> sortItems = new ArrayList<>();
Review Comment:
```suggestion
List<OrderKey> orderKeys = new ArrayList<>();
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -351,15 +339,12 @@ private LogicalPlan withAggClause(List<NamedExpression> aggExpressions,
* @param ctx SortItemContext
* @return SortItems
*/
- public SortItems genSortItems(SortItemContext ctx) {
- OrderDirection orderDirection;
- if (ctx.DESC() != null) {
- orderDirection = OrderDirection.DESC;
- } else {
- orderDirection = OrderDirection.ASC;
- }
+ public OrderKey genSortItems(SortItemContext ctx) {
Review Comment:
```suggestion
public OrderKey genOrderKeys(SortItemContext ctx) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalHeapSortToPhysicalHeapSort.java:
##########
@@ -0,0 +1,36 @@
+// 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.PhysicalHeapSort;
+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 sort to physical sort.
+ */
+public class LogicalHeapSortToPhysicalHeapSort extends OneImplementationRuleFactory {
Review Comment:
```suggestion
public class LogicalSortToPhysicalHeapSort extends OneImplementationRuleFactory {
```
##########
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;
Review Comment:
Please add the comments about `partitionExprList `
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/PhysicalPlanTranslator.java:
##########
@@ -147,20 +148,20 @@ public PlanFragment visitPhysicalOlapScan(
}
@Override
- public PlanFragment visitPhysicalSort(PhysicalUnaryPlan<PhysicalSort, Plan> sort,
+ public PlanFragment visitPhysicalSort(PhysicalUnaryPlan<PhysicalHeapSort, Plan> sort,
PlanTranslatorContext context) {
PlanFragment childFragment = visit(sort.child(0), context);
- PhysicalSort physicalSort = sort.getOperator();
+ PhysicalHeapSort physicalHeapSort = sort.getOperator();
if (!childFragment.isPartitioned()) {
Review Comment:
Why doesn't the sort node translate if the childfragment is a single instance?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalAggregation.java:
##########
@@ -88,10 +89,7 @@ public <R, C> R accept(PlanOperatorVisitor<R, C> visitor, Plan plan, C context)
@Override
public List<Expression> getExpressions() {
- return new ImmutableList.Builder<Expression>()
- .addAll(groupByExprList)
- .addAll(aggExprList)
- .addAll(partitionExprList)
- .build();
+ return new ImmutableList.Builder<Expression>().addAll(groupByExprList).addAll(outputExpressionList)
+ .addAll(partitionExprList).build();
Review Comment:
partitionExprList maybe null
##########
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> outputExpressionList;
+ 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> outputExpressionList) {
super(OperatorType.LOGICAL_AGGREGATION);
- this.groupByExpressions = groupByExpressions;
- this.outputExpressions = outputExpressions;
+ this.groupByExprList = groupByExprList;
+ this.outputExpressionList = outputExpressionList;
}
- /**
- * Get GroupByAggregation list.
- *
- * @return all group by of this node.
- */
- public List<Expression> getGroupByExpressions() {
- return groupByExpressions;
+ public List<Expression> getPartitionExprList() {
+ return partitionExprList;
Review Comment:
```suggestion
return partitionExprList == null ? groupByExprList : partitionExprList;
```
--
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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
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-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
[GitHub] [doris] EmmyMiao87 commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [doris] EmmyMiao87 commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r908229632
##########
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;
Review Comment:
I agree with you ~
--
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
[GitHub] [doris] github-actions[bot] commented on pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10335:
URL: https://github.com/apache/doris/pull/10335#issuecomment-1168446804
PR approved by at least one committer and no changes requested.
--
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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r903593652
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalOlapScan.java:
##########
@@ -0,0 +1,21 @@
+package org.apache.doris.nereids.operators.plans.logical;
Review Comment:
header
--
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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r906970105
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalHeapSort.java:
##########
@@ -32,37 +32,29 @@
/**
* 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;
-
+ private final List<OrderKey> orderKeys;
private final boolean useTopN;
Review Comment:
useTopN need to be removed. In translator, HeapSort will translate to topN and MergeSort will translate to normal 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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r907274021
##########
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;
Review Comment:
i think the `partitionExprList` just copy from stale planner, may be we do not need it anymore in future
--
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
[GitHub] [doris] github-actions[bot] commented on pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10335:
URL: https://github.com/apache/doris/pull/10335#issuecomment-1168446872
PR approved by anyone and no changes requested.
--
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
[GitHub] [doris] yinzhijian commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
yinzhijian commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r904730468
##########
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;
}
- /**
- * Get GroupByAggregation list.
- *
- * @return all group by of this node.
- */
- public List<Expression> getGroupByExpressions() {
- return groupByExpressions;
+ public List<Expression> getPartitionExprList() {
+ return partitionExprList;
}
- /**
- * Get outputExpressions list.
- *
- * @return all agg expressions.
- */
- public List<? extends NamedExpression> getoutputExpressions() {
- return outputExpressions;
+ public void setPartitionExprList(List<Expression> partitionExprList) {
+ this.partitionExprList = partitionExprList;
+ }
+
+ public List<Expression> getGroupByExprList() {
+ return groupByExprList;
+ }
+
+ public List<NamedExpression> getAggExprList() {
+ return aggExprList;
+ }
+
+ public AggPhase getAggPhase() {
+ return aggPhase;
}
@Override
public String toString() {
- return "Aggregation (" + "outputExpressions: " + StringUtils.join(outputExpressions, ", ")
- + ", groupByExpressions: " + StringUtils.join(groupByExpressions, ", ") + ")";
+ return "Aggregation (" + "outputExpressions: " + StringUtils.join(aggExprList, ", ") + ", groupByExpressions: "
Review Comment:
Is it better to unify it with the variable name here？outputExpressions=>aggExprList, etc.
--
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
[GitHub] [doris] EmmyMiao87 commented on pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 commented on PR #10335:
URL: https://github.com/apache/doris/pull/10335#issuecomment-1163897976
It is recommended to change the useless List<? extends A> in the code to List<A>
--
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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r903492537
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalProjectionToPhysicalProjection.java:
##########
@@ -23,7 +23,7 @@
import org.apache.doris.nereids.trees.plans.Plan;
/**
- * Implementation rule that convert logical join to physical hash join.
+ * Implementation rule that convert logical projection to physical projection.
Review Comment:
```suggestion
* Implementation rule that convert logical project to physical project.
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalAggregation.java:
##########
@@ -31,68 +32,82 @@
/**
* 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<? extends NamedExpression> aggExprList;
+ private List<Expression> partitionExprList;
+ private AggPhase aggPhase;
Review Comment:
i think use different agg and group by expr is enough to distinguish different agg phase. We need to remove it later
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/AbstractOperator.java:
##########
@@ -27,16 +27,16 @@
*/
public abstract class AbstractOperator implements Operator {
protected final OperatorType type;
- protected final long limited;
+ protected long limit;
Review Comment:
all operator should immutable, if need to change limit, we should add a function named `withLimit` or use full parameter constructor
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalAggregation.java:
##########
@@ -31,68 +32,82 @@
/**
* 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<? extends NamedExpression> aggExprList;
+ private List<Expression> partitionExprList;
+ private AggPhase aggPhase;
+ private boolean usingStream;
Review Comment:
logical don't need this
--
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
[GitHub] [doris] EmmyMiao87 commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r907220207
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/RuleType.java:
##########
@@ -42,12 +42,12 @@ public enum RuleType {
LOGICAL_JOIN_EXCHANGE(RuleTypeClass.EXPLORATION),
// implementation rules
+ LOGICAL_AGG_TO_HASH_AGG_RULE(RuleTypeClass.IMPLEMENTATION),
LOGICAL_JOIN_TO_HASH_JOIN_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_PROJECTION_TO_PHYSICAL_PROJECTION_RULE(RuleTypeClass.IMPLEMENTATION),
+ LOGICAL_PROJECT_TO_PHYSICAL_PROJECT_RULE(RuleTypeClass.IMPLEMENTATION),
LOGICAL_FILTER_TO_PHYSICAL_FILTER_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_LIMIT_TO_PHYSICAL_LIMIT_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_JOIN_TO_HASH_AGG_RULE(RuleTypeClass.IMPLEMENTATION),
- LOGICAL_JOIN_TO_OLAP_SCAN_RULE(RuleTypeClass.IMPLEMENTATION),
+ LOGICAL_HEAP_SORT_TO_PHYSICAL_HEAP_SORT_RULE(RuleTypeClass.IMPLEMENTATION),
Review Comment:
```suggestion
LOGICAL_SORT_TO_PHYSICAL_HEAP_SORT_RULE(RuleTypeClass.IMPLEMENTATION),
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalAggToHashAgg.java:
##########
@@ -0,0 +1,43 @@
+// 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 {
Review Comment:
```suggestion
public class LogicalAggToPhysicalHashAgg extends OneImplementationRuleFactory {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/PhysicalPlanTranslator.java:
##########
@@ -95,7 +96,7 @@ public PlanFragment visitPhysicalAggregation(
ArrayList<Expr> execGroupingExpressions = groupByExpressionList.stream()
.map(e -> ExpressionConverter.convert(e, context)).collect(Collectors.toCollection(ArrayList::new));
- List<Expression> aggExpressionList = physicalAggregation.getAggExprList();
+ List<NamedExpression> aggExpressionList = physicalAggregation.getOutputExpressionList();
Review Comment:
```suggestion
List<NamedExpression> outputExpressionList = physicalAggregation.getOutputExpressionList();
```
--
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
[GitHub] [doris] morrySnow commented on a diff in pull request #10335: [enhancement](Nereids): add more implmentation rules.
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10335:
URL: https://github.com/apache/doris/pull/10335#discussion_r907290138
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/PhysicalPlanTranslator.java:
##########
@@ -147,20 +148,20 @@ public PlanFragment visitPhysicalOlapScan(
}
@Override
- public PlanFragment visitPhysicalSort(PhysicalUnaryPlan<PhysicalSort, Plan> sort,
+ public PlanFragment visitPhysicalSort(PhysicalUnaryPlan<PhysicalHeapSort, Plan> sort,
PlanTranslatorContext context) {
PlanFragment childFragment = visit(sort.child(0), context);
- PhysicalSort physicalSort = sort.getOperator();
+ PhysicalHeapSort physicalHeapSort = sort.getOperator();
if (!childFragment.isPartitioned()) {
Review Comment:
it is a bug introduced by @Kikyou1997, please fix it~
--
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