----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30466/#review70841 -----------------------------------------------------------
PojoRecordReader needs infinite affinity. Please fix and add test case with slice target as 1 and group by/sort to force exchange insertion and make sure that infinite affinity works correctly (maybe add a few more cases on just that sub functionality) Why don’t we always have small affinity to previous node for other exchanges? exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java <https://reviews.apache.org/r/30466/#comment116282> I'm not sure what this method name means. Maybe something more like isAffinityRequired() exec/java-exec/src/main/java/org/apache/drill/exec/physical/EndpointAffinity.java <https://reviews.apache.org/r/30466/#comment116888> This seems misnamed. Maybe isAffinityRequired() exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractExchange.java <https://reviews.apache.org/r/30466/#comment116889> Probably this should be a constant rather than constantly recreating the default. exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/GroupScan.java <https://reviews.apache.org/r/30466/#comment116894> It seems ParallelizationInfo and these other methods are redundant. Why do we have both? Also wondering why you deleted HasAffinity. The idea was that any node could have affinity and that we ultimately choose to understand affinity based on that. exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/Receiver.java <https://reviews.apache.org/r/30466/#comment116895> If we are using this all over, let's use IntObjectOpenHashMap<DrillbitEndpoint> exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/DeMuxExchange.java <https://reviews.apache.org/r/30466/#comment116898> IllegalState? exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/DeMuxExchange.java <https://reviews.apache.org/r/30466/#comment116897> Why not use a ArrayListMultiMap? exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/DeMuxExchange.java <https://reviews.apache.org/r/30466/#comment116912> Naming this senderDrillbitLocation is confusing. This is the location of the receiver. Yes, you're building a map that is sendendpoint -> [recv, recv] but I still found this confusing. exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/broadcastsender/BroadcastSenderRootExec.java <https://reviews.apache.org/r/30466/#comment116984> Why did you change to Map? exec/java-exec/src/main/java/org/apache/drill/exec/planner/fragment/Wrapper.java <https://reviews.apache.org/r/30466/#comment116992> This naming is a bit misleading. It sounds like whether or not this will be parallel but I believe it is actually have we already made parallelization decisions about this. If the latter, we should give a more descriptive name. exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/DefaultSqlHandler.java <https://reviews.apache.org/r/30466/#comment116994> Let's make mux and demux separate configuration options. exec/java-exec/src/main/java/org/apache/drill/exec/work/batch/AbstractDataCollector.java <https://reviews.apache.org/r/30466/#comment116995> Why are you changing to maps everywhere? Is this because you now have Sparse lists? I'm not sure it is worth the change. If you must, let's create an array wrapper class that doesn't do hashing. exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java <https://reviews.apache.org/r/30466/#comment117010> Should this be in basetest exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java <https://reviews.apache.org/r/30466/#comment117011> can we move this functionality to base test query? exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java <https://reviews.apache.org/r/30466/#comment117012> can we move this to common class? exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestLocalExchange.java <https://reviews.apache.org/r/30466/#comment117013> can we move to common? - Jacques Nadeau On Feb. 4, 2015, 4:30 p.m., Venki Korukanti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30466/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2015, 4:30 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/AbstractReceiver.java > f621a26 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractSender.java > 53a0721 > > 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/PhysicalOperatorUtil.java > dfcb113 > > 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/Sender.java > bbd1b2c > 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/BroadcastSender.java > 1827367 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/DeMuxExchange.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/HashPartitionSender.java > bdb1362 > > 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/OrderedPartitionSender.java > 0a2b9be > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/RangeSender.java > c8c8f43 > > 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/physical/impl/broadcastsender/BroadcastSenderRootExec.java > 22fa047 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionSenderRootExec.java > f09acaa > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/partitionsender/PartitionerTemplate.java > 4292c09 > > 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/DeMuxExchangePrel.java > PRE-CREATION > > 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/direct/DirectGroupScan.java > aa1609d > > exec/java-exec/src/main/java/org/apache/drill/exec/store/ischema/InfoSchemaGroupScan.java > 8335ed9 > > exec/java-exec/src/main/java/org/apache/drill/exec/store/mock/MockGroupScanPOP.java > 5736df8 > > 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/store/sys/SystemTableScan.java > 053f5de > > 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/PopUnitTestBase.java > 9a32ff9 > > 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 > >
