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