> On Aug. 15, 2015, 5:49 p.m., Aman Sinha wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java, > > line 39 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040486#file1040486line39> > > > > It's a little odd to have LAG specific member variable in a class that > > is shared by all window functions. A derived class would be better.
True. The current implementation of DefaultFrameTemplate become too complex because it needs to handle all supported window functions. I am planing on refactoring this class into multiple subclasses that will handle different "families" of window functions, it will be probably part of another patch though > On Aug. 15, 2015, 5:49 p.m., Aman Sinha wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java, > > line 142 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040486#file1040486line142> > > > > Similarly for FIRST_VALUE...should this go to a derived class since we > > don't want to do this for all window functions ? same as above > On Aug. 15, 2015, 5:49 p.m., Aman Sinha wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFunction.java, > > line 95 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040491#file1040491line95> > > > > You might want to change this to WindowAggregate.. done - abdelhakim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37482/#review95521 ----------------------------------------------------------- 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 > >
