drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aman Sinha" <asi...@maprtech.com>
Subject Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions
Date Sun, 16 Aug 2015 02:49:23 GMT

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



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java
(line 136)
<https://reviews.apache.org/r/37482/#comment150557>

    Can a single partition exceed 2^31 rows ? If so, the length should be a long.  Is there
any validation done inside computePartitionSize() for this ?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java
(line 302)
<https://reviews.apache.org/r/37482/#comment150558>

    We should not be calling setup methods inside a loop.  This should ideally be done once
for a record batch otherwise the performance will be poor.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java
(line 310)
<https://reviews.apache.org/r/37482/#comment150559>

    Same as above.



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java
(line 228)
<https://reviews.apache.org/r/37482/#comment150555>

    The collector.hasErrors() is checked later..shouldn't it be checked right after calling
materialize ?



exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFunction.java
(line 260)
<https://reviews.apache.org/r/37482/#comment150556>

    I think you should check for input == NULL after calling materialize since an expression
materializer may return null under normal circumstances.   Also, the collector could be holding
an error state which should be checked.  Alaternatively, you could use ExpressionTreeMaterializer.materializeAndCheckErrors().
    
    This applies to other window functions also..


- Aman Sinha


On Aug. 14, 2015, 6:40 p.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37482/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2015, 6:40 p.m.)
> 
> 
> Review request for drill and Aman Sinha.
> 
> 
> Bugs: DRILL-3536
>     https://issues.apache.org/jira/browse/DRILL-3536
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> - added support for the new functions in DefaultFrameTemplate
> - use of an internal batch buffer to store values between batches when computing LAG
> - added new aggregate function "holdLast" to store intermediate values between batches
when computing FIRST_VALUE
> - added unit tests for the new functions
> - fixed DRILL-3604, 3605 and 3606
> - GenerateTestData is an internal tool used to generate data files and their expected
results for window function unit tests
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java
535deaa 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/Partition.java
8d6728e 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowDataBatch.java
5045cb3 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java
9c8cfc0 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFramer.java
69866af 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFunction.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java
04d1231 
>   exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 9e09106

>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/GenerateTestData.java
PRE-CREATION 
>   exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/TestWindowFrame.java
553c4e8 
>   exec/java-exec/src/test/resources/window/3604.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3605.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3605.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3606.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3606.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3648.parquet PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3648.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/3648.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b2.p4.ntile.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.fval.pby.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.lag.oby.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.lag.pby.oby.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.lead.oby.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.lead.pby.oby.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.lval.pby.oby.tsv PRE-CREATION 
>   exec/java-exec/src/test/resources/window/b4.p4.oby.tsv 528f2f3 
>   exec/java-exec/src/test/resources/window/b4.p4.pby.oby.tsv a5d630b 
>   exec/java-exec/src/test/resources/window/b4.p4.pby.tsv b2bd5e1 
>   exec/java-exec/src/test/resources/window/b4.p4.tsv 1731fe9 
>   exec/java-exec/src/test/resources/window/b4.p4/0.data.json e91a75c 
>   exec/java-exec/src/test/resources/window/b4.p4/1.data.json 52f375b 
>   exec/java-exec/src/test/resources/window/b4.p4/2.data.json 9ecc5ed 
>   exec/java-exec/src/test/resources/window/b4.p4/3.data.json 32d2ad1 
>   exec/java-exec/src/test/resources/window/fewRowsAllData.parquet PRE-CREATION 
>   exec/java-exec/src/test/resources/window/fval.alltypes.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/fval.pby.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/lag.oby.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/lag.pby.oby.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/lead.oby.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/lead.pby.oby.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/lval.alltypes.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/lval.pby.oby.sql PRE-CREATION 
>   exec/java-exec/src/test/resources/window/ntile.sql PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37482/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


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