> On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java, > > line 38 > > <https://reviews.apache.org/r/30466/diff/1/?file=842691#file842691line38> > > > > This class has no member variables? Perhaps the constructor should be > > private so that no one else creates more instances, and they are always > > forced to use the singleton?
Added private constructor. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/ParallelizationInfo.java, > > line 32 > > <https://reviews.apache.org/r/30466/diff/1/?file=842692#file842692line32> > > > > It looks like the affinityMap can be final. Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/PlanningSet.java, > > line 30 > > <https://reviews.apache.org/r/30466/diff/1/?file=842693#file842693line30> > > > > It looks like the fragmentMap should be final. Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java, > > line 77 > > <https://reviews.apache.org/r/30466/diff/1/?file=842694#file842694line77> > > > > Put the result of context.getOptions() into a local and reuse that > > throughout this function. Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java, > > line 204 > > <https://reviews.apache.org/r/30466/diff/1/?file=842694#file842694line204> > > > > Put stats.getParallelizationInfo() into a local, and then reuse that > > throughout the rest of this function. Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java, > > line 257 > > <https://reviews.apache.org/r/30466/diff/1/?file=842694#file842694line257> > > > > Why does this need to be sorted? What is the sort key? We want to order the EndpointAffinity based in descending order of affinity values. DrillbitEndpoints with high affinity get picked up first when selecting the endpoints. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java, > > line 261 > > <https://reviews.apache.org/r/30466/diff/1/?file=842694#file842694line261> > > > > This is a little unusual, because most code would normally not expect > > the size of a collection to change like this, and many would use > > final int count = endpoints.size(); > > while(count < ....) { > > ... > > } > > A comment here would be really helpful, something like "keep adding > > endpoints until we have the same number as the slot count." Added a comment. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java, > > line 59 > > <https://reviews.apache.org/r/30466/diff/1/?file=842697#file842697line59> > > > > Can this be final? Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java, > > line 63 > > <https://reviews.apache.org/r/30466/diff/1/?file=842697#file842697line63> > > > > Can this be final? Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java, > > line 223 > > <https://reviews.apache.org/r/30466/diff/1/?file=842697#file842697line223> > > > > this. isn't necessary here. Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/AbstractDataCollector.java, > > line 57 > > <https://reviews.apache.org/r/30466/diff/1/?file=842704#file842704line57> > > > > Doesn't need this. Changed. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java, > > line 52 > > <https://reviews.apache.org/r/30466/diff/1/?file=842710#file842710line52> > > > > What is this commented out query? Should it just be deleted? Removed. Also in the v2 patch, this class has more tests. > On Feb. 3, 2015, 12:03 a.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java, > > line 59 > > <https://reviews.apache.org/r/30466/diff/1/?file=842710#file842710line59> > > > > What is this commented out line? Removed. - Venki ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30466/#review70639 ----------------------------------------------------------- On Feb. 2, 2015, 7:21 p.m., Venki Korukanti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30466/ > ----------------------------------------------------------- > > (Updated Feb. 2, 2015, 7:21 p.m.) > > > Review request for drill, Chris Westin, Jacques Nadeau, and Steven Phillips. > > > Repository: drill-git > > > Description > ------- > > In this patch LocalExchange contains only multiplexing exchange. Currently > working on demultiplexing exchange (there are few failures in executions and > currently debugging those). Sending this patch to get initial feedback. > > Brief overview of changes: > 1. Traverse the PRel tree after all optimizations. Whenever a > HashToRandomExchangePrel is encountered insert a MuxExchange before > HashToRandomExchangePrel and DemuxExchange after HashToRandomExchangePrel. > 2. Parallelization changes: > i) Traverse the physical operator tree and divide it into Fragments. > ii) Based on the affinity of the sending Exchange, set parallelization > dependencies between fragments. > iii) Start parallelizing from the leaf fragments (fragment that have no > other fragments depending on them for parallelization info). Stats collection > include collecting parallelization info (minWidth, maxWidth, affinityMap) and > cost. > 3. Change the Receiver to accept set of (minorFragmentId, DrillbitEndpoint) > as sender list. This also involved few changes in DataCollector. > 4. Change SingleSender to accept custom minorFragmentId instead of default > minorFragmentId of 0 > > > Diffs > ----- > > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java > df31f74 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractExchange.java > 73280ea > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScan.java > 5d0d9bf > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Exchange.java > 7be7f20 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/GroupScan.java > 23860a3 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/HasAffinity.java > 52462db > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Receiver.java > 0c67770 > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Store.java > 94411ea > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/BroadcastExchange.java > 73a1d20 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashToMergeExchange.java > f62d922 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashToRandomExchange.java > fac374b > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergingReceiverPOP.java > f5dca1a > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MuxExchange.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/OrderedPartitionExchange.java > 8e1526a > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/Screen.java > 58c8e29 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleMergeExchange.java > 2914112 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/SingleSender.java > 4a11a51 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnionExchange.java > cfc21ac > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java > 3a4dd0e > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/SingleSenderCreator.java > 6db9f4a > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Fragment.java > ac63bde > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/MakeFragmentsVisitor.java > 8756e5b > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java > 961b603 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/ParallelizationInfo.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/PlanningSet.java > 8cc6c85 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java > 434cdd4 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Stats.java > eda364b > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/StatsCollector.java > 41ff678 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java > 86b395e > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/MuxExchangePrel.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java > faa8546 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java > 79603eb > > exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java > f20627d > > exec/java-exec/src/main/java/org/apache/drill/exec/store/mock/MockStorePOP.java > 4c12d57 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/AbstractDataCollector.java > c83106c > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/DataCollector.java > dc016be > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/IncomingBuffers.java > b0206f7 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/MergingCollector.java > ce14260 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/PartitionedCollector.java > 5190d84 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > b33042b > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/pop/TestFragmentChecker.java > 6349b76 > > Diff: https://reviews.apache.org/r/30466/diff/ > > > Testing > ------- > > Ran Functional and TPCH SF100 parquet verification tests. > > > Thanks, > > Venki Korukanti > >
