----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35584/#review88321 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java (line 64) <https://reviews.apache.org/r/35584/#comment140783> This might not be related to this JIRA. But I feel this "for" loop is problemtic. Are we going to support the following: avg(c_1) over (partition by c_2 order by c_3), sum(c_2) over (partition by c_4 order by c_5) Seems each different group will re-set the traits based on different "partition"/"order" keys, which would cause incorrect problem. If we do not support, then we probably should add assertion check to make sure there is only one group in window. exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java (line 99) <https://reviews.apache.org/r/35584/#comment140778> Line 99 uses windowBase.orderKeys, while line 181 uses window.collation(). It took me a while to figure out they refer to the same thing. Is it better to use one form, to avoid confusion? exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java (line 100) <https://reviews.apache.org/r/35584/#comment140780> Why do u need call convert() over exch? the input has been converted, and the SingleMergeExchangePrel is explicitly created in this rule. Sounds it's not necessary to call convert() again. Also, you may consider adding one unit test case for the failed query reported in the JIRA. - Jinfeng Ni On June 17, 2015, 4:59 p.m., Aman Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35584/ > ----------------------------------------------------------- > > (Updated June 17, 2015, 4:59 p.m.) > > > Review request for drill and Jinfeng Ni. > > > Bugs: DRILL-3298 > https://issues.apache.org/jira/browse/DRILL-3298 > > > Repository: drill-git > > > Description > ------- > > The JIRA DRILL-3298 has relevant discussion on this. The fix involves > creating a single stream as input to the Window by inserting a > SingleMergeExchange if only the ORDER-BY clause is present in a Window. This > ensures that the Sort below the Window is done in parallel followed by a > Merge. > > > Diffs > ----- > > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java > f7728c8 > > Diff: https://reviews.apache.org/r/35584/diff/ > > > Testing > ------- > > Manual testing of the query in DRILL-3298. Ran unit tests. Functional tests > are in progress. > > > Thanks, > > Aman Sinha > >
