drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Westin" <chriswesti...@gmail.com>
Subject Re: Review Request 30965: Follow up on DRILL-133 (LocalExchange) to save CPU cycles on hash generation when using in HashToLocalExchange
Date Mon, 02 Mar 2015 23:09:33 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message