You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/11/03 13:55:29 UTC
[GitHub] [calcite] chunweilei commented on a change in pull request #2237: [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first
chunweilei commented on a change in pull request #2237:
URL: https://github.com/apache/calcite/pull/2237#discussion_r515812551
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -311,31 +312,39 @@ default boolean onProduce(RelNode node) {
}
if (group.taskState != null && upperBound.isLe(group.upperBound)) {
- // either this group failed to optimize before or it is a ring
+ // Either this group failed to optimize before or it is a ring.
return;
}
group.startOptimize(upperBound);
- // cannot decide an actual lower bound before MExpr are fully explored
- // so delay the lower bound checking
+ // Cannot decide an actual lower bound before MExpr are fully explored.
+ // So delay the lower bound checking.
- // a gate keeper to update context
+ // A gate keeper to update context.
tasks.push(new GroupOptimized(group));
- // optimize mExprs in group
+ // Optimize mExprs in group.
List<RelNode> physicals = new ArrayList<>();
for (RelNode rel : group.set.rels) {
if (planner.isLogical(rel)) {
tasks.push(new OptimizeMExpr(rel, group, false));
} else if (rel.isEnforcer()) {
- // Enforcers have lower priority than other physical nodes
+ // Enforcers have lower priority than other physical nodes.
physicals.add(0, rel);
} else {
physicals.add(rel);
}
}
- // always apply O_INPUTS first so as to get an valid upper bound
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
+ // It only happens when enabling AbstractConverter.
+ if (group.getConvention() == Convention.NONE) {
+ LOGGER.debug("Skip optimizing because of none convention: {}", group);
+ return;
+ }
Review comment:
@vlsi Here is the main change.
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
It means OptimzeInput, which is a term in Cascades planner. I borrow it from the comment[1].
[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java#L338
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
I would change it as you suggest. But the term O_INPUT is not invented currently. It comes from a classic paper[1] which was published in 1998.
[1] https://15721.courses.cs.cmu.edu/spring2019/papers/22-optimizer1/xu-columbia-thesis1998.pdf
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
physicals.add(rel);
}
}
+
+ // No need to apply O_INPUTS if the group has not been implemented yet.
Review comment:
I would change it as you suggest. But the term O_INPUT is not invented currently. It comes from a classic paper[1] which was published in 1998.
[1] https://15721.courses.cs.cmu.edu/spring2019/papers/22-optimizer1/xu-columbia-thesis1998.pdf Page#63
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org