drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdelhakim deneche" <adene...@gmail.com>
Subject Re: Review Request 37482: DRILL-3536: Add support for LEAD, LAG, NTILE, FIRST_VALUE and LAST_VALUE window functions
Date Tue, 18 Aug 2015 15:51:21 GMT


> On Aug. 16, 2015, 2:49 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java,
line 309
> > <https://reviews.apache.org/r/37482/diff/3/?file=1040486#file1040486line309>
> >
> >     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.
> 
> abdelhakim deneche wrote:
>     when aggregated all peer rows we may need to process multiple batches, setupEvaluatePeer()
will be called once per batch in order for evaluatePeer() to read the values from the correct
batch.
>     
>     Yet, setupEvaluatePeer() may still be called multiple times if we have lot's of small
frames per batch. I should be able to "fix" this as part of a follow-up patch, especially
after I refactor the Template into separate classes
> 
> Aman Sinha wrote:
>     Yes, lots of small frames per batch is a likely scenario so we should address that.
 Separate follow-up patch sounds ok to me.
> 
> Aman Sinha wrote:
>     Could you pls file a JIRA for this item (avoid calling setupEvaluatePeer or related
setup methods more than once per batch) and the other pending item with DefaultFrameTemplate
?  Thanks.

Done

https://issues.apache.org/jira/browse/DRILL-3662


- abdelhakim


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


On Aug. 18, 2015, 3:25 p.m., abdelhakim deneche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37482/
> -----------------------------------------------------------
> 
> (Updated Aug. 18, 2015, 3:25 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
> -------
> 
> unit tests are passing along with functional and tpch100
> 
> 
> Thanks,
> 
> abdelhakim deneche
> 
>


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