Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/797#discussion_r109025164
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/SubsetTransformer.java
---
@@ -45,15 +48,20 @@ public RelTraitSet newTraitSet(RelTrait... traits) {
}
- boolean go(T n, RelNode candidateSet) throws E {
+ public boolean go(T n, RelNode candidateSet) throws E {
if ( !(candidateSet instanceof RelSubset) ) {
return false;
}
boolean transform = false;
+ Set<RelNode> transformedRels = Sets.newHashSet();
for (RelNode rel : ((RelSubset)candidateSet).getRelList()) {
if (isPhysical(rel)) {
RelNode newRel = RelOptRule.convert(candidateSet,
rel.getTraitSet().plus(Prel.DRILL_PHYSICAL));
+ if(transformedRels.contains(newRel)) {
--- End diff --
Change to use newIdentityHashSet.
And we could not simply mark a node is isTransformed, since there could be
many rules transforming the same node. Maintaining a map of applied rules in
relNode might not be a good idea since remembering what rule has been applied
should not be the job of a relNode(set). So what about maintaining such map in
planner? Not a good idea since the same node might need to be converted into
different sets. So I would think leaving this part as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
