Re: Review Request 30965: Follow up on DRILL-133 (LocalExchange) to save CPU cycles on hash generation when using in HashToLocalExchange
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30965/#review74844
---
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121627
Why are groupByMultipleQuery, groupByMutipleQueryBaselineColumns, and
groupByMultipleQueryBaselineValues members of the test class instead of final
locals?
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121630
final
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121631
final
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121628
Since you don't use groupByMultipleQueryBaselineValues until here, can't
you just move the loop above down to here and just use
for(int i = 0; i NUM_DEPTS; i++) {
testBuilder.baselineValues(new Object[] { (long)i, (long)0, (long)0,
(long)numOccurrances});
}
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121629
No blank line needed here.
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121646
This does more than just parse JSON. A name like jsonMuxChecker or
something similar would be better.
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121632
can jsonP, planObj, and graphArray be final?
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121633
It looks like the variables you initialize at the top of each of these
nested blocks can be final.
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121649
MUX_EXCHANGE, as defined above? You used HASH_EXCHANGE below Similarly
for the DEMUX_EXCHANGE down below.
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
https://reviews.apache.org/r/30965/#comment121650
Leave off the does not match expected; JUnit will say that if the actual
doesn't match the expected.
- Chris Westin
On March 2, 2015, 1:56 p.m., Yuliya Feldman wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30965/
---
(Updated March 2, 2015, 1:56 p.m.)
Review request for drill, Jacques Nadeau, Jinfeng Ni, Steven Phillips, and
Venki Korukanti.
Bugs: DRILL-2209
https://issues.apache.org/jira/browse/DRILL-2209
Repository: drill-git
Description
---
Insert Project operator to add new column EXPRHASH with hash expression for
fields that are used for HashToRandomExchange
Remove Project operator after HashRandomExchange (or Demux) since it will
create problems to fields ordering in HashJoin.
Tight this to MuxExchange - so if MuxExchange is enabled, Project is inserted.
Diffs
-
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToRandomExchangePrel.java
372c75d
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java
1adc54f
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java
PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java
PRE-CREATION
Diff: https://reviews.apache.org/r/30965/diff/
Testing
---
Need to add Unit Tests. tested live, run Functional and TPCH tests
Thanks,
Yuliya Feldman
Re: Review Request 30965: Follow up on DRILL-133 (LocalExchange) to save CPU cycles on hash generation when using in HashToLocalExchange
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30965/ --- (Updated Feb. 26, 2015, 12:34 a.m.) Review request for drill, Jacques Nadeau, Jinfeng Ni, Steven Phillips, and Venki Korukanti. Changes --- Addressing review comments after code review with Jacques and Venki Bugs: DRILL-2209 https://issues.apache.org/jira/browse/DRILL-2209 Repository: drill-git Description --- Insert Project operator to add new column EXPRHASH with hash expression for fields that are used for HashToRandomExchange Remove Project operator after HashRandomExchange (or Demux) since it will create problems to fields ordering in HashJoin. Tight this to MuxExchange - so if MuxExchange is enabled, Project is inserted. Diffs (updated) - exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToRandomExchangePrel.java 372c75d exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PrelUtil.java 1adc54f exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java PRE-CREATION Diff: https://reviews.apache.org/r/30965/diff/ Testing --- Need to add Unit Tests. tested live, run Functional and TPCH tests Thanks, Yuliya Feldman
Re: Review Request 30965: Follow up on DRILL-133 (LocalExchange) to save CPU cycles on hash generation when using in HashToLocalExchange
On Feb. 23, 2015, 5:44 p.m., Jinfeng Ni wrote: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java, line 112 https://reviews.apache.org/r/30965/diff/2/?file=871428#file871428line112 MuxExchange has Project as its child. So, MuxExchange will have same traits as Project (addColumnprojectPrel), in stead of its parent (prel). Will definitely fix it - thank you for pointing out On Feb. 23, 2015, 5:44 p.m., Jinfeng Ni wrote: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java, line 127 https://reviews.apache.org/r/30965/diff/2/?file=871428#file871428line127 I'm not fully clear about the motification of inserting the hash expression into Project. But here if we remove the compuated hash expression, does it mean that the down stream operator will not be able to refer to this computed value, and have to re-compute? The problem is that if we have HashJoin later on it is not aware of additional column and it will be failing, so after discussion with Jacques we decided to add Project before HashExchage and remove it after - so to thw world outside of Mux/HashExchange/Demux it will look as Project was never inserted - Yuliya --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30965/#review73732 --- On Feb. 23, 2015, 4:09 p.m., Yuliya Feldman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30965/ --- (Updated Feb. 23, 2015, 4:09 p.m.) Review request for drill, Jacques Nadeau, Jinfeng Ni, Steven Phillips, and Venki Korukanti. Bugs: DRILL-2209 https://issues.apache.org/jira/browse/DRILL-2209 Repository: drill-git Description --- Insert Project operator to add new column EXPRHASH with hash expression for fields that are used for HashToRandomExchange Remove Project operator after HashRandomExchange (or Demux) since it will create problems to fields ordering in HashJoin. Tight this to MuxExchange - so if MuxExchange is enabled, Project is inserted. Diffs - exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToRandomExchangePrel.java 372c75d exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java PRE-CREATION Diff: https://reviews.apache.org/r/30965/diff/ Testing --- Need to add Unit Tests. tested live, run Functional and TPCH tests Thanks, Yuliya Feldman
Re: Review Request 30965: Follow up on DRILL-133 (LocalExchange) to save CPU cycles on hash generation when using in HashToLocalExchange
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30965/#review73231 --- exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java https://reviews.apache.org/r/30965/#comment119494 Won't prel.getCluster().getRexBuilder() be the same for every pass of the loop body? Do it once before and reuse. Same for child.getRowType().getFieldList(). (And child.getRowType() could be reused up above as well). In this line, you call field.getFieldId() twice, when you could call it once before this line, and then use that value twice. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java https://reviews.apache.org/r/30965/#comment119495 More use of prel.getCluster().getRexBuilder(). exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java https://reviews.apache.org/r/30965/#comment119496 More use of prel.getCluster().getRexBuilder(). exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java https://reviews.apache.org/r/30965/#comment119497 prel.getCluster() here and down below. - Chris Westin On Feb. 12, 2015, 9:29 p.m., Yuliya Feldman wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30965/ --- (Updated Feb. 12, 2015, 9:29 p.m.) Review request for drill, Jacques Nadeau, Steven Phillips, and Venki Korukanti. Bugs: DRILL-2209 https://issues.apache.org/jira/browse/DRILL-2209 Repository: drill-git Description --- Insert Project operator to add new column EXPRHASH with hash expression for fields that are used for HashToRandomExchange Remove Project operator after HashRandomExchange (or Demux) since it will create problems to fields ordering in HashJoin. Tight this to MuxExchange - so if MuxExchange is enabled, Project is inserted. Diffs - exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/HashToRandomExchangePrel.java 372c75d exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java PRE-CREATION Diff: https://reviews.apache.org/r/30965/diff/ Testing --- Need to add Unit Tests. tested live, run Functional and TPCH tests Thanks, Yuliya Feldman
