drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aman Sinha" <asi...@maprtech.com>
Subject Re: Review Request 35584: DRILL-3298: fix wrong result for window function query when no partition-by is present
Date Thu, 18 Jun 2015 19:11:46 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message