Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9777 )
Change subject: IMPALA-4025: Part 1: Add percentile_disc aggregation function ...................................................................... Patch Set 4: (5 comments) add a tosql test to exercise that code path. http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java: http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@160 PS4, Line 160: fn.setIsPersistent(true); why is this set to true? http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@247 PS4, Line 247: public TFunction toThrift() { logical fns should never escape the fe, so should we poison the cases where they could get out? http://gerrit.cloudera.org:8080/#/c/9777/4/testdata/workloads/functional-query/queries/QueryTest/aggregation.test File testdata/workloads/functional-query/queries/QueryTest/aggregation.test: http://gerrit.cloudera.org:8080/#/c/9777/4/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1372 PS4, Line 1372: remove the blank line http://gerrit.cloudera.org:8080/#/c/9777/4/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1553 PS4, Line 1553: # Percentile with subquery rewrite Add a case where a constant is on LHS, e.g., instead of int_col not in ... -> 10 not in ... That tests interaction between two rewrites in stmt rewriter. http://gerrit.cloudera.org:8080/#/c/9777/4/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1558 PS4, Line 1558: from alltypesagg); lets add tests for: - correlated subquery - outer join (percentile over resulting, nullable columns) - case where ordering column values are all null or contain null - percentile is specified as part of a stored view definition do we have a flag for controlling null first/last when sorting (I assume its only specified at the query/clause level)? -- To view, visit http://gerrit.cloudera.org:8080/9777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacef7b3fcd74c4c73d88400ce27307c3baa0121e Gerrit-Change-Number: 9777 Gerrit-PatchSet: 4 Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 20 Apr 2018 18:50:38 +0000 Gerrit-HasComments: Yes
