drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hanifi Gunes" <hanifigu...@gmail.com>
Subject Re: Review Request 32945: DRILL-2715: Implement nested loop join operator
Date Wed, 08 Apr 2015 22:55:00 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32945/#review79434
-----------------------------------------------------------


Some initial comments.


exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/ExpandableHyperContainerContext.java
<https://reviews.apache.org/r/32945/#comment128792>

    final?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/ExpandableHyperContainerContext.java
<https://reviews.apache.org/r/32945/#comment128791>

    Using recordCounts.clear() would be better here.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinBatch.java
<https://reviews.apache.org/r/32945/#comment128801>

    static?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinBatch.java
<https://reviews.apache.org/r/32945/#comment128800>

    static?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinBatch.java
<https://reviews.apache.org/r/32945/#comment128793>

    we should consider making left & right final. Also a null check here would be nice.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinBatch.java
<https://reviews.apache.org/r/32945/#comment128798>

    should be final. also can we use longer names here?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128810>

    is this instance variable needed when we are holding left instance? can we localize?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128814>

    the same. isn't this equal to cardinality of rightCounts? localize?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128823>

    we should localize this as well.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128815>

    localize?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128816>

    localize?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128817>

    localize?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128819>

    use of local variables would be more *performant* here.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
<https://reviews.apache.org/r/32945/#comment128821>

    Afaik any non-long integer arithmetic results in an int by default. We may save some cpu
cycles if we declare nextRightRecordToProcess be short and get rid of bit masking here.
    
    Also is there any reason for not altering method signature to emitRight(compositeIndex,
recordIndex, outIndex) instead of making this computation for each tuple?


- Hanifi Gunes


On April 8, 2015, 12:27 a.m., Mehant Baid wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32945/
> -----------------------------------------------------------
> 
> (Updated April 8, 2015, 12:27 a.m.)
> 
> 
> Review request for drill and Aman Sinha.
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> This patch implements the nested loop join operator. The main changes are in the files
NestedLoopJoinBatch and NestedLoopJoinTemplate. This patch only contains the execution changes.
Planning patch will be posted in a separate review request by Aman.
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/AbstractPhysicalVisitor.java
27b0ecb 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalVisitor.java
e6a89d0 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/NestedLoopJoinPOP.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/ExpandableHyperContainerContext.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoin.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinBatch.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinBatchCreator.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/NestedLoopJoinTemplate.java
PRE-CREATION 
>   protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java 9a9d196 
>   protocol/src/main/protobuf/UserBitShared.proto 5e44655 
> 
> Diff: https://reviews.apache.org/r/32945/diff/
> 
> 
> Testing
> -------
> 
> The tests are dependent on the planning changes, hence not uploaded as part of this patch.
However https://github.com/mehant/drill/blob/notin_1/exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/join/TestNestedLoopJoin.java
is a working branch that contains a bunch of tests (planning & execution) added for nested
loop join.
> 
> 
> Thanks,
> 
> Mehant Baid
> 
>


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