caicancai commented on code in PR #3851:
URL: https://github.com/apache/calcite/pull/3851#discussion_r1678632932
##########
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java:
##########
@@ -982,11 +982,25 @@ void propagateCostImprovements(RelNode rel) {
if (!relNode.getTraitSet().satisfies(subset.getTraitSet())) {
continue;
}
- if (!cost.isLt(subset.bestCost)) {
+
+ // Update subset best and best's cost when we find a cheaper rel
+ if (relNode != subset.best && !cost.isLt(subset.bestCost)) {
continue;
}
- // Update subset best cost when we find a cheaper rel or the current
- // best's cost is changed
+
+ // Update the RelNode's cost when we find that the cost is changed.
+ //
+ // Why do we need to update it?
+ // When RelSet's one of the subsets find a cheaper RelNode, we need to
update the
+ // parents of the subset to have the best RelNode and best cost.
+ // In theory, this cost will become smaller, But according to
+ // The SQL we added in the JdbcAdapterTest {@link
testVolcanoPlannerInternalValid},
+ // it shows RelNode's cost will become bigger sometimes.
+ // So we update it.
Review Comment:
I'm not sure if the keyword "we" is appropriate to describe
##########
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java:
##########
@@ -982,11 +982,25 @@ void propagateCostImprovements(RelNode rel) {
if (!relNode.getTraitSet().satisfies(subset.getTraitSet())) {
continue;
}
- if (!cost.isLt(subset.bestCost)) {
+
+ // Update subset best and best's cost when we find a cheaper rel
+ if (relNode != subset.best && !cost.isLt(subset.bestCost)) {
continue;
}
- // Update subset best cost when we find a cheaper rel or the current
- // best's cost is changed
+
+ // Update the RelNode's cost when we find that the cost is changed.
+ //
+ // Why do we need to update it?
+ // When RelSet's one of the subsets find a cheaper RelNode, we need to
update the
+ // parents of the subset to have the best RelNode and best cost.
+ // In theory, this cost will become smaller, But according to
+ // The SQL we added in the JdbcAdapterTest {@link
testVolcanoPlannerInternalValid},
Review Comment:
The SQL -> the SQL
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
