Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3210: last/first_value() support for IGNORE NULLS ......................................................................
Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/3328/7/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java File fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java: Line 592: * 4. UNBOUNDED FOLLOWING windows: combine 3 and 4, it looks like the same transformation is applied Line 661: // 3. see above http://gerrit.cloudera.org:8080/#/c/3328/7/fe/src/main/java/com/cloudera/impala/catalog/BuiltinsDb.java File fe/src/main/java/com/cloudera/impala/catalog/BuiltinsDb.java: Line 491: private static final Map<Type, String> LAST_VALUE_IGNORE_NULLS_INIT_SYMBOL = instead of registering a new (wrapper) function, why not simply have the existing last_value() take an extra bool parameter? (which you would then have to supply during analyze()) Line 979: prefix + UPDATE_VAL_SYMBOL.get(t), is update_val_symbol used anywhere else? http://gerrit.cloudera.org:8080/#/c/3328/7/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test File testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test: Line 1555: | window: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW why is this one not coalesced w/ the parent analytic node? for first_value, both windows are the same, so it'd be good to normalize here. Line 2187: # Start LAST_VALUE IGNORE NULLS is there a reason this needs new test cases (or could this be incorporated into existing test cases)? Line 2190: last_value(tinyint_col ignore nulls) over (order by id rows between unbounded preceding and 1 preceding) long line (and elsewhere) -- 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: 7 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
