FatLittle commented on a change in pull request #1991:
URL: https://github.com/apache/calcite/pull/1991#discussion_r436255565
##########
File path: core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
##########
@@ -317,8 +284,8 @@ RelSubset getOrCreateSubset(
subset.setDelivered();
}
- if (needsConverter && !planner.topDownOpt) {
- addConverters(subset, required, true);
+ if (needsConverter) {
+ addConverters(subset, required, !planner.topDownOpt);
Review comment:
Yes, it is. There are reasons for adding converters eagerly:
1. The converters act as bridges between the working subset and other
subsets. Currently, the OptimizeGroup task will schedule tasks for all the
logical Rels in the RelSet while it skips physical Rels from other subsets.
That's because we are confident that if other subset can be reached by a
converter, the converter is already in the current working subset.
2. In tasks other than OptimizeGroup, when new physical Rels are added to
the RelSet, the insertion of the converter is a signal for RuleDriver to
schedule the optimizing tasks for the previous working subset.
3. The computation of lower bound requires the memo to be connected.
Maybe there are some other ways to achieve all this, but I cannot find a
more pretty way now. I was thinking of checking the upper bound before adding
the enforcer. But I cannot decide which place is suitable for adding the
enforcer back when the upper bound is changed. So I leave it for further
optimizations.
----------------------------------------------------------------
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:
[email protected]
