drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Hsuan-Yi Chu" <hsua...@usc.edu>
Subject Re: Review Request 35365: DRILL-3182, DRILL-3188, DRILL-3195
Date Thu, 11 Jun 2015 23:43:17 GMT


> On June 11, 2015, 11:20 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java,
line 120
> > <https://reviews.apache.org/r/35365/diff/1/?file=982877#file982877line120>
> >
> >     Instead of 'disabled' we should be consistent with the error messages and say
'The window function <name> is not currently supported <rest of the message>'

A new message:
"The window function " + functionName + " is not supported\n"


> On June 11, 2015, 11:20 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java,
line 134
> > <https://reviews.apache.org/r/35365/diff/1/?file=982877#file982877line134>
> >
> >     Change to 'DISTINCT for window aggregate functions is not currently supported...'
 (since DISTINCT only applies to window aggregates not to rank, dense_rank etc.)

Update according to the comment


> On June 11, 2015, 11:20 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java,
line 158
> > <https://reviews.apache.org/r/35365/diff/1/?file=982877#file982877line158>
> >
> >     Instead of using string comparisons, we should use the methods SqlWindow.isCurrentRow(),
SqlWindow.isUnboundedPreceding() etc.  or the enum Bound.

Update according to the comment


> On June 11, 2015, 11:20 p.m., Aman Sinha wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java, line
105
> > <https://reviews.apache.org/r/35365/diff/1/?file=982878#file982878line105>
> >
> >     On the latest master branch, the window.enable option is now set to true, so
you can remove these from the unit tests.

Update according to the comment


> On June 11, 2015, 11:20 p.m., Aman Sinha wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java, line
148
> > <https://reviews.apache.org/r/35365/diff/1/?file=982878#file982878line148>
> >
> >     For conciseness, it would be better to have a single test which has a query
string which takes various types of windows functions that are unsupported (lead, lag etc.)
and just runs them one by one.  This will reduce the duplication. In any case, in a subsequent
release we will be supporting these functions.

Sounds concise. But in these cases, the correct behavior is "seeing an expected exception",
which junit would validate this.

For sure, we can use a while loop to wrap try-catch to take over junit's job. But would that
presents some confusion ?


- Sean Hsuan-Yi


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35365/#review87639
-----------------------------------------------------------


On June 11, 2015, 8:33 p.m., Sean Hsuan-Yi Chu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35365/
> -----------------------------------------------------------
> 
> (Updated June 11, 2015, 8:33 p.m.)
> 
> 
> Review request for drill, Aman Sinha and Jinfeng Ni.
> 
> 
> Bugs: DRILL-3182, DRILL-3188 and DRILL-3195
>     https://issues.apache.org/jira/browse/DRILL-3182
>     https://issues.apache.org/jira/browse/DRILL-3188
>     https://issues.apache.org/jira/browse/DRILL-3195
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Window function with DISTINCT qualifier is disabled; Disable unsupported window frames;
Window functions NTILE, LAG, LEAD, FIRST_VALUE, LAST_VALUE are disabled
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java
b92de3b 
>   exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java fc75d73

> 
> Diff: https://reviews.apache.org/r/35365/diff/
> 
> 
> Testing
> -------
> 
> All the required tests
> 
> 
> Thanks,
> 
> Sean Hsuan-Yi Chu
> 
>


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