[ 
https://issues.apache.org/jira/browse/DRILL-5286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15942070#comment-15942070
 ] 

ASF GitHub Bot commented on DRILL-5286:
---------------------------------------

Github user paul-rogers commented on a diff in the pull request:

    https://github.com/apache/drill/pull/797#discussion_r108048071
  
    --- 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 --
    
    Does this work? A hash map depends on `hashCode()` and `equals()`. A random 
check of one `DrillRelNode`, `DrillFilterRelBase`, shows that the above two 
methods are not implemented. As a result, set membership may be based on object 
identity. But, surely each call to `convert()` will create a new object?
    
    Or, does this rule apply to only certain kinds of nodes which do have 
`hashCode()` and `equals()` implemented? If so, perhaps add some comments to 
identify the subtree of nodes with which this code works.
    
    Do we have a test case for this code? Or, did you step through it to see if 
it was doing what we want? If so, and it worked, then likely this code does 
work with a specific subset of nodes that have the needed methods.


> When rel and target candidate set is the same, planner should not need to do 
> convert for the relNode since it must have been done
> ---------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: DRILL-5286
>                 URL: https://issues.apache.org/jira/browse/DRILL-5286
>             Project: Apache Drill
>          Issue Type: Bug
>            Reporter: Chunhui Shi
>            Assignee: Chunhui Shi
>




--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Reply via email to