impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3210: last/first value() support for IGNORE NULLS
Date Wed, 13 Jul 2016 23:09:22 GMT
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 <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