> On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java, > > line 33 > > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line33> > > > > It looks like endpoint should be final.
updated. > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java, > > line 45 > > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line45> > > > > 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? This method is not needed. Removed it. > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java, > > line 54 > > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line54> > > > > What does it mean for the two endpoint values not to be equal? How do > > you expect this to be used? We use this for sorting the EndpointAffinity list based on the affinity. Added a comment. > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java, > > line 66 > > <https://reviews.apache.org/r/30466/diff/1/?file=842669#file842669line66> > > > > 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)? We use POSITIVE_INIFINITY to indicate that endpoint in this object should have an assignment. One example is Screen. Fragment containing Screen should always be assigned to the node where the Foreman for the query is present. > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractExchange.java, > > line 29 > > <https://reviews.apache.org/r/30466/diff/1/?file=842670#file842670line29> > > > > 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. These are set when assigning the sender endpoint list in AbstractExchange.setupSenders() (similar method for receiver) > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractGroupScan.java, > > line 52 > > <https://reviews.apache.org/r/30466/diff/1/?file=842671#file842671line52> > > > > 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? We are returning the empty list which is a static final in Collections. Most of the implementations of GroupScan cache the result and don't recalculate everytime getOperatorAffinity() method is called (I see some exceptions, I am going to address later as this problem is not introduced by this change). > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MergingReceiverPOP.java, > > line 58 > > <https://reviews.apache.org/r/30466/diff/1/?file=842680#file842680line58> > > > > 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; > > } The reason why are using get(i) is because we want to create a mapping of "i" -> DrillbitEndpoint. DrillbitEndpoint assigned to minor fragment 3 is at index 3 in the list. > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/MuxExchange.java, > > line 131 > > <https://reviews.apache.org/r/30466/diff/1/?file=842681#file842681line131> > > > > "this." isn't necessary here. > > > > I don't see any code that actually sets this variable. Removed this. It is set in AbstractExchange.setupSenders/Receivers() > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java, > > line 38 > > <https://reviews.apache.org/r/30466/diff/1/?file=842687#file842687line38> > > > > Why don't we make this map final? changed. > On Feb. 2, 2015, 10:18 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/UnorderedReceiver.java, > > line 46 > > <https://reviews.apache.org/r/30466/diff/1/?file=842687#file842687line46> > > > > 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? Moved it to Abstract Receiver. - Venki ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30466/#review70618 ----------------------------------------------------------- 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 > >
