----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30466/#review70639 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Fragment.java <https://reviews.apache.org/r/30466/#comment115923> But addOperator() above will silently drop this on the floor if root is already set. Should that give an error similar to the above? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Materializer.java <https://reviews.apache.org/r/30466/#comment115924> 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? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/ParallelizationInfo.java <https://reviews.apache.org/r/30466/#comment115925> It looks like the affinityMap can be final. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/PlanningSet.java <https://reviews.apache.org/r/30466/#comment115926> It looks like the fragmentMap should be final. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115927> Put the result of context.getOptions() into a local and reuse that throughout this function. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115928> Given how you're using it, roots should be a HashSet. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115930> Put stats.getParallelizationInfo() into a local, and then reuse that throughout the rest of this function. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115931> Move the declaration of this list down to just before the while() loop where it is used, below. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115932> Why do you create the affinedEPs copy of the endpointAffinityMap values, when you could just use endpointAffinityMap.values() directly here? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115933> Why does this need to be sorted? What is the sort key? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115934> 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." exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115935> Removing random items from a list is going to cause a list traversal for each item in the list. In this case, it would be better to create "all" as an empty list, and then iterate over activeEndpoints, and only add it to all if it is not in the endpointAffinityMap() (which is a hash lookup to test). exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115936> Don't create this empty list if we don't need it. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115937> Don't create this empty list if we don't need it. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/SimpleParallelizer.java <https://reviews.apache.org/r/30466/#comment115939> I don't understand what this loop is doing. It seems to leave sendingEndpoints set to the last parallelized sendingFragment's endpoints. What is significant about the last one it finds? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java <https://reviews.apache.org/r/30466/#comment115951> Can this be final? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java <https://reviews.apache.org/r/30466/#comment115952> Can this be final? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java <https://reviews.apache.org/r/30466/#comment115949> this. isn't necessary here. exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java <https://reviews.apache.org/r/30466/#comment115950> Would it be dangerous for the caller to modify this? exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/AbstractDataCollector.java <https://reviews.apache.org/r/30466/#comment115953> Doesn't need this. exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java <https://reviews.apache.org/r/30466/#comment115959> What is this commented out query? Should it just be deleted? exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java <https://reviews.apache.org/r/30466/#comment115960> What is this commented out line? - Chris Westin 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 > >
