Matthew Jacobs has posted comments on this change. Change subject: IMPALA-1882: Remove ORDER BY restriction from first_value()/last_value() ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7502/1//COMMIT_MSG Commit Message: PS1, Line 14: Analyser nit: Analyzer to be consistent with the actual code naming conventions http://gerrit.cloudera.org:8080/#/c/7502/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java: PS1, Line 422: boolean isFirstOrLastValueFn = fnName.equals(LAST_VALUE) : || fnName.equals(FIRST_VALUE) || fnName.equals(LAST_VALUE_IGNORE_NULLS) : || fnName.equals(FIRST_VALUE_IGNORE_NULLS); make this a function like we do above for isXFn(), e.g. isOffsetFn(). No need for a local variable. Reasons: 1) consistency with the other function types 2) fewer local variables is better, assuming we don't need to worry about perf (and we don't here) PS1, Line 454: isFirstOrLastValueFn then call that fn inline here PS1, Line 475: isFirstOrLastValueFn also call the fn here http://gerrit.cloudera.org:8080/#/c/7502/1/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test File testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test: Line 2012 Nice deterministic tests :) Can you add test cases that exercise IGNORE NULLS as well? You should be able to use alltypesagg in a similar way. -- To view, visit http://gerrit.cloudera.org:8080/7502 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5a3a56833ac062839629353ea240b361bc727d96 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
