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/04 02:51:25 UTC
[GitHub] [calcite] chunweilei opened a new pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
chunweilei opened a new pull request #2243:
URL: https://github.com/apache/calcite/pull/2243
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517080225
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -69,8 +69,8 @@
private GeneratorTask applying = null;
/**
- * RelNodes that are generated by passThrough or derive
- * these nodes will not takes part in another passThrough or derive.
+ * RelNodes that are generated by passThrough or derive.
+ * These nodes will not take part in another passThrough or derive.
Review comment:
It would be helpful to add some links to the `passThrough` and `derive` operations.
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei merged pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
chunweilei merged pull request #2243:
URL: https://github.com/apache/calcite/pull/2243
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517085070
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -311,31 +311,31 @@ 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 check.
- // 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
+ // Always apply O_INPUTS first so as to get an valid upper bound.
Review comment:
Done.
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -170,27 +170,27 @@ private void clearProcessed(RelSet set) {
}
}
- // a callback invoked when a RelNode is going to be added into a RelSubset,
- // either by Register or Reregister. The task driver should need to schedule
- // tasks for the new nodes.
+ // A callback invoked when a RelNode is going to be added into a RelSubset,
+ // either by Register or Reregister. The task driver should schedule tasks
+ // for the new nodes.
@Override public void onProduce(RelNode node, RelSubset subset) {
- // if the RelNode is added to another RelSubset, just ignore it.
- // It should be schedule in the later OptimizeGroup task
+ // If the RelNode is added to another RelSubset, just ignore it.
+ // It should be scheduled in the later OptimizeGroup task.
if (applying == null || subset.set
!= VolcanoPlanner.equivRoot(applying.group().set)) {
return;
}
- // extra callback from each task
+ // Extra callback from each task.
if (!applying.onProduce(node)) {
return;
}
if (!planner.isLogical(node)) {
// For a physical node, schedule tasks to optimize its inputs.
// The upper bound depends on all optimizing RelSubsets that this Rel belongs to.
Review comment:
Done.
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517083461
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -601,7 +604,7 @@ private boolean checkLowerBound(RelNode rel, RelSubset group) {
}
/**
- * A task that optimize input for physical nodes who has only one input.
+ * A task that optimizes input for physical nodes who has only one input.
* This task can be replaced by OptimizeInputs but simplify lots of logic.
Review comment:
simplify -> simplifies
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517081499
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -170,27 +170,27 @@ private void clearProcessed(RelSet set) {
}
}
- // a callback invoked when a RelNode is going to be added into a RelSubset,
- // either by Register or Reregister. The task driver should need to schedule
- // tasks for the new nodes.
+ // A callback invoked when a RelNode is going to be added into a RelSubset,
+ // either by Register or Reregister. The task driver should schedule tasks
+ // for the new nodes.
@Override public void onProduce(RelNode node, RelSubset subset) {
- // if the RelNode is added to another RelSubset, just ignore it.
- // It should be schedule in the later OptimizeGroup task
+ // If the RelNode is added to another RelSubset, just ignore it.
+ // It should be scheduled in the later OptimizeGroup task.
if (applying == null || subset.set
!= VolcanoPlanner.equivRoot(applying.group().set)) {
return;
}
- // extra callback from each task
+ // Extra callback from each task.
if (!applying.onProduce(node)) {
return;
}
if (!planner.isLogical(node)) {
// For a physical node, schedule tasks to optimize its inputs.
// The upper bound depends on all optimizing RelSubsets that this Rel belongs to.
Review comment:
Rel -> RelNode?
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
chunweilei commented on pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#issuecomment-721499337
@liyafan82 thank you for your review. All comments are addressed.
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517082361
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -311,31 +311,31 @@ 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 check.
- // 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
+ // Always apply O_INPUTS first so as to get an valid upper bound.
Review comment:
an -> 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517083286
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -590,7 +593,7 @@ private RelNode convert(RelNode rel, RelSubset group) {
return null;
}
- // check whether a node's lower bound is less than a RelSubset's upper bound
+ // Check whether a node's lower bound is less than a RelSubset's upper bound.
Review comment:
Change this to a JavaDoc, and "Check" -> "Checks"?
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517085147
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -69,8 +69,8 @@
private GeneratorTask applying = null;
/**
- * RelNodes that are generated by passThrough or derive
- * these nodes will not takes part in another passThrough or derive.
+ * RelNodes that are generated by passThrough or derive.
+ * These nodes will not take part in another passThrough or derive.
Review comment:
Sounds reasonable.
----------------------------------------------------------------
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
[GitHub] [calcite] liyafan82 commented on a change in pull request #2243: Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2243:
URL: https://github.com/apache/calcite/pull/2243#discussion_r517083852
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java
##########
@@ -768,22 +772,22 @@ private boolean checkLowerBound(RelNode rel, RelSubset group) {
@Override public void perform() {
if (input != parent.getInput(i)) {
- // The input has chnaged. So reschedule the optimize task.
+ // The input has changed. So reschedule the optimize task.
input = (RelSubset) parent.getInput(i);
tasks.push(this);
tasks.push(new OptimizeGroup(input, upper));
return;
}
- // Optimize input completed. Update the context for other inputs
+ // Optimize input completed. Update the context for other inputs.
Review comment:
Optimize -> Optimizing
----------------------------------------------------------------
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