> 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 139 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040486#file1040486line139> > > > > 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 ?
Good point, will change length to long > 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. 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 > 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 317 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040486#file1040486line317> > > > > Same as above. setupReadLastValue() will only be called for the last peer row. I will move this outside the loop > On Aug. 16, 2015, 2:49 a.m., Aman Sinha wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFrameRecordBatch.java, > > line 298 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040489#file1040489line298> > > > > The collector.hasErrors() is checked later..shouldn't it be checked > > right after calling materialize ? Good point, will fix > On Aug. 16, 2015, 2:49 a.m., Aman Sinha wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFunction.java, > > line 260 > > <https://reviews.apache.org/r/37482/diff/3/?file=1040491#file1040491line260> > > > > 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.. fixed - abdelhakim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37482/#review95533 ----------------------------------------------------------- On Aug. 17, 2015, 3:54 p.m., abdelhakim deneche wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37482/ > ----------------------------------------------------------- > > (Updated Aug. 17, 2015, 3:54 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 > >
