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