> On June 18, 2015, 1:11 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrule.java, > > line 64 > > <https://reviews.apache.org/r/35584/diff/1/?file=986627#file986627line64> > > > > 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. > > Aman Sinha wrote: > Currently your query will fail with an unsupported operation error: > > Error: UNSUPPORTED_OPERATION ERROR: Multiple window definitions in a > single SELECT list is not currently supported > See Apache Drill JIRA: DRILL-3196 > > I am not sure if resetting the traits in the loop would cause incorrect > results; it would be suboptimal. There is a comment at the beginning: > // TODO: Order window based on existing partition by > //input.getTraitSet().subsumes() > > Jinfeng Ni wrote: > Right. The UNSUPPORTED_OPERATION ERROR is what I expect. If we do not > allow multiple window definitions, then seems this for loop in WindowPrule is > not necessary. In stead, we could simply check if window.groups has size = 1. > Something like: > > Preconditions.checkArgument(window.groups.size() == 1, "Only one > window definition is allowed."); > > Window.Group windowBase = window.groups.get(0);
I discussed this offline with Jinfeng and we agreed that the current logic in the for loop will produce valid plans. In fact, please see a discussion of this in DRILL-3196 where both the plan and results were validated for multiple window partitions. The reason we are currently blocking this functionality is because of lack of sufficient testing, not due to implementation. - Aman ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35584/#review88321 ----------------------------------------------------------- On June 18, 2015, 7:07 p.m., Aman Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35584/ > ----------------------------------------------------------- > > (Updated June 18, 2015, 7:07 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 > exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java > 2ec2481 > > 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 > >
