impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3210: last/first value() support for IGNORE NULLS
Date Wed, 06 Jul 2016 20:10:53 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message