drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Nadeau" <jacques.dr...@gmail.com>
Subject Re: Review Request 30466: DRILL-133: LocalExchange (planning and parallelization)
Date Thu, 05 Feb 2015 23:04:20 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message