Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3210: last/first_value() support for IGNORE NULLS ......................................................................
Patch Set 11: (5 comments) Nice! I think we can get this in very soon, I'd like to make sure we have some decimal tests though. Also, the EE functional tests look like you've thought through a lot of cases - thanks! While the query gen support for Oracle might not land right now, it would be nice to see if you can manually check some of those results against the oracle db. We can find out from Michael Brown if he can share his oracle instance and whether or not he's managed to load tables yet. http://gerrit.cloudera.org:8080/#/c/3328/11/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java File fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java: PS11, Line 565: to return the last value. We either set : * the fn to be 'last_value' or 'first_value_rewrite', which simply wraps the : * 'last_value' implementation but allows us to handle the first rows in a partition : * in a special way in the backend I'd vote to remove this text (which I think was already here before) as the details are explained in the cases below. PS11, Line 671: "last_value" LAST_VALUE PS11, Line 747: nit: extra spaces here? Whatever is consistent with other code in this file. http://gerrit.cloudera.org:8080/#/c/3328/11/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test File testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test: Line 1617: 07:ANALYTIC > We currently only group analytic fns in the same node if their order by is Yes I think we could add this as an optimization for ROWS windows. Thomas, would you mind filing a JIRA for that? Also, it would be nice to add a test case demonstrating 2 analytic fns that will now be coalesced that weren't before-- I don't think we have that here. http://gerrit.cloudera.org:8080/#/c/3328/11/testdata/workloads/functional-query/queries/QueryTest/decimal.test File testdata/workloads/functional-query/queries/QueryTest/decimal.test: new test cases for decimal? -- To view, visit http://gerrit.cloudera.org:8080/3328 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic27525e2237fb54318549d2674f1610884208e9b Gerrit-PatchSet: 11 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
