----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30466/#review70618 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java <https://reviews.apache.org/r/30466/#comment115906> It looks like endpoint should be final. exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java <https://reviews.apache.org/r/30466/#comment115909> If I created this for a particular endpoint, why would I want to be able to change it? If I do change it, why doesn't that invalidate the affinity value? exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java <https://reviews.apache.org/r/30466/#comment115910> What does it mean for the two endpoint values not to be equal? How do you expect this to be used? exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java <https://reviews.apache.org/r/30466/#comment115856> Can this condition ever occur? It it actually possible to reach POSITIVE_INFINITY by adding to the affinity as in addAffinity() (and the condition in there also looks suspicious)? exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractExchange.java <https://reviews.apache.org/r/30466/#comment115915> Why are these not final (and set in an AbstractExchange constructor)? Is the MuxExchange below, the sender major fragment id is referenced, but has never been set. It looks like these aren't both used. Perhaps they shouldn't be declared in this class, but each class that does need them should declare whichever one it needs itself. exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScan.java <https://reviews.apache.org/r/30466/#comment115920> This will make a new (empty) list each time it is called. Shouldn't the class be hanging on to its currently known operator affinity list, even an empty one? exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Exchange.java <https://reviews.apache.org/r/30466/#comment115896> It would be helpful to expand on this comment with some of the reasons these might have affinity with each other. In my mind, they would always want to be together (if possible) to avoid any network transit, so I'm having a hard time with finding a reason they wouldn't want to have affinity. exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergingReceiverPOP.java <https://reviews.apache.org/r/30466/#comment115903> From these declarations, we can't tell what kind of List senders is; it might be an ArrayList, or it might be a LinkedList. In the latter case, get(i) is expensive, because it will iterate down the list to get to each item. Because of that, we should iterate over that list instead, something like this: int i = 0; for(DrillbitEndpoint de : senders) { this.senders.put(i, de); ++i; } exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MuxExchange.java <https://reviews.apache.org/r/30466/#comment115914> "this." isn't necessary here. I don't see any code that actually sets this variable. exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java <https://reviews.apache.org/r/30466/#comment115917> Why don't we make this map final? exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java <https://reviews.apache.org/r/30466/#comment115918> This code is the same as that in MergingReceiverPOP (which has one additional different line at its end); should it be pulled up into AbstractReceiver? exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java <https://reviews.apache.org/r/30466/#comment115919> How does this get used? Would it be dangerous if the caller modified the map? Should we return a copy? - 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 > >
