----------------------------------------------------------- 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 > >
