Aaaaaaron commented on a change in pull request #2222:
URL: https://github.com/apache/calcite/pull/2222#discussion_r513342587
##########
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##########
@@ -190,7 +190,8 @@ protected void onMatch() {
return;
}
- if (subset.set.equivalentSet != null) {
+ if ((subset.set.equivalentSet != null)
+ || (subset != rel && !subset.getRelList().contains(rel))) {
Review comment:
@liyafan82 I dig the code, and it's not about equivalentSet, let me
explain to you:
1. In RelSet#mergeWith, we ill rename parent rels. note, what we renamed is
the equivalentSet's parent, so it may not have equivalentSet(child have
equivalentSet)
```
// Update all rels which have a child in the other set, to reflect the
// fact that the child has been renamed.
//
// Copy array to prevent ConcurrentModificationException.
final List<RelNode> previousParents =
ImmutableList.copyOf(otherSet.getParentRels());
for (RelNode parentRel : previousParents) {
planner.rename(parentRel);
}
```
2. in VolcanoPlanner#rename(RelNode rel), we found equivRel for the rel, we
will remove the rel from the subset
```
// Remove rel from its subset. (This may leave the subset
// empty, but if so, that will be dealt with when the sets
// get merged.)
final RelSubset subset = mapRel2Subset.put(rel, equivRelSubset);
```
that's why I add this condition
----------------------------------------------------------------
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:
us...@infra.apache.org
