[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[Rahul Challapalli (JIRA)]

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

Rahul Challapalli commented on DRILL-5159:
--

[~jni] Can you guide me into coming up with a functional test which exposes 
this issue?

> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Jinfeng Ni
>  Labels: ready-to-commit
> Fix For: 1.10.0
>
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



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

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[Kunal Khatua (JIRA)]

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

Kunal Khatua commented on DRILL-5159:
-

[~rkins] Please close the bug when you have tests automated .

> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Jinfeng Ni
>  Labels: ready-to-commit
> Fix For: 1.10.0
>
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



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

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[ASF GitHub Bot (JIRA)]

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

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

Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/705


> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Jinfeng Ni
>  Labels: ready-to-commit
> Fix For: 1.10.0
>
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[ASF GitHub Bot (JIRA)]

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

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

Github user amansinha100 commented on the issue:

https://github.com/apache/drill/pull/705
  
+1


> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Aman Sinha
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[ASF GitHub Bot (JIRA)]

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

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

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/705#discussion_r94443444
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillMergeProjectRule.java
 ---
@@ -48,6 +50,12 @@ public boolean matches(RelOptRuleCall call) {
 Project topProject = call.rel(0);
 Project bottomProject = call.rel(1);
 
+// Make sure both projects be LogicalProject.
+if (topProject.getTraitSet().getTrait(ConventionTraitDef.INSTANCE) != 
Convention.NONE ||
--- End diff --

You are right that the current Drill specific MergeProjectRule extends from 
Calcite's version and the only difference is to check complex output 
expression. Therefore, it make senses the currently Drill MergeProjectRule 
operates on Calcite logical convention only. 

Yes, in the future, we would consider allowing merging 2 DrillProjectRels. 
That should be another instance of rule, which operates on DRILL_LOGICAL 
convention only.  


> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Aman Sinha
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[ASF GitHub Bot (JIRA)]

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

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

Github user amansinha100 commented on a diff in the pull request:

https://github.com/apache/drill/pull/705#discussion_r94437725
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillMergeProjectRule.java
 ---
@@ -48,6 +50,12 @@ public boolean matches(RelOptRuleCall call) {
 Project topProject = call.rel(0);
 Project bottomProject = call.rel(1);
 
+// Make sure both projects be LogicalProject.
+if (topProject.getTraitSet().getTrait(ConventionTraitDef.INSTANCE) != 
Convention.NONE ||
--- End diff --

At first glance, it is non-intuitive that a Drill specific MergeProjectRule 
returns False if a DRILL_LOGICAL convention is encountered.  However, 
currently, this rule only checks for the complex expression (e.g 
convert_to_json), so your change is fine in the context of the current rule.  
In future, would we consider allowing merging 2 DrillProjectRels ? (I think 
yes). 


> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Aman Sinha
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[Jinfeng Ni (JIRA)]

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

Jinfeng Ni commented on DRILL-5159:
---

[~amansinha100], could you please review this PR? thanks!


> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Jinfeng Ni
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-5159) ProjectMergeRule in Drill should operate on RelNodes with same convention trait.

[ASF GitHub Bot (JIRA)]

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

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

GitHub user jinfengni opened a pull request:

https://github.com/apache/drill/pull/705

DRILL-5159: Make sure Drill's ProjectMergeRule operate on RelNodes wi…

…th same convention trait.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jinfengni/incubator-drill DRILL-5159

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/705.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #705


commit 02af67c6223689e952cbe4775ccc6dec28269ada
Author: Jinfeng Ni 
Date:   2016-12-22T02:00:46Z

DRILL-5159: Make sure Drill's ProjectMergeRule operate on RelNodes with 
same convention trait.




> ProjectMergeRule in Drill should operate on RelNodes with same convention 
> trait.
> 
>
> Key: DRILL-5159
> URL: https://issues.apache.org/jira/browse/DRILL-5159
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Reporter: Jinfeng Ni
>Assignee: Jinfeng Ni
>
> Drill extended version of  Calcite's ProjectMergeRule is used in a 
> VolcanoPlanner where RelNodes with different convention could match with this 
> rule. 
> For instance, we could see this rule could be invoked when a DrillProject on 
> top of a LogicalProject. Also, since the output RelNode is built from the 
> default Project RelFactory, such rule execution could end up with a 
> LogicalProject.
> {code}
> DrillProjecttransform  
> \  ===>   LogicalProject
> LogicalProject
> {code}
>  
> This leads to un-necessary rule execution, or in certain case could lead to 
> an infinite loop.  
> The proposed fix is to check matched RelNodes to make sure that they do have 
> Calcite Logical convention. That way, both inputs and output of this rule 
> would have same convention trait.  This should reduce planning time, and 
> avoid the possiblity of loop.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)