Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21976 )
Change subject: IMPALA-13481: Add support for various agg and analytic functions ...................................................................... Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/21976/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RewriteRexOverRule.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RewriteRexOverRule.java: http://gerrit.cloudera.org:8080/#/c/21976/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RewriteRexOverRule.java@121 PS12, Line 121: * = ((Count - Rank) + 1)/Count : * where, : * Rank = rank() over([partition by clause] order by clause DESC) : * Count = count() over([partition by clause]) This seems to be the same as Rank / Count where Rank = rank() over ([partition by clause] order by clause) Count = count() over ([partition by clause] If we formulate it that way, we don't need to flip anything. (I realize we're matching the original formula over in AnalyticExpr.java, but I do think they are equivalent unless I'm missing some corner case.) http://gerrit.cloudera.org:8080/#/c/21976/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RewriteRexOverRule.java@127 PS12, Line 127: Set<SqlKind> descendingSet = ImmutableSet.of(SqlKind.DESCENDING); Nit: descendingSet is unused http://gerrit.cloudera.org:8080/#/c/21976/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RewriteRexOverRule.java@130 PS12, Line 130: List<RexFieldCollation> descendingCollation = new ArrayList<>(); : for (RexFieldCollation collation : over.getWindow().orderKeys) { : descendingCollation.add(new RexFieldCollation(collation.getKey(), descendingSet)); : } : ImmutableList allDescendingCollation = ImmutableList.copyOf(descendingCollation); Nit: descendingCollation and allDescendingCollation are unused http://gerrit.cloudera.org:8080/#/c/21976/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RewriteRexOverRule.java@239 PS12, Line 239: List<RexFieldCollation> tmpReversed = Lists.reverse(collationList); I don't think we want to reverse this list. If we have (A asc, B asc, C asc, D asc, E asc), we want (A desc, B desc, C desc, D desc, E desc) not (E desc, D desc, C desc, B desc, A desc). http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test File testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test: http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1391 PS12, Line 1391: # Test percent_rank() desc with multiple partition exprs and multiple order by exprs and : # compare with the rewrite target query Nit: Can we add a sentence before this saying this is a copy of the previous test case with the order direction flipped? http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1394 PS12, Line 1394: rank() over (partition by year, month order by id) Nit: These statements are formulated so that the last two columns are the same. One is percent_rank() while the other is the equivalent formula. To keep this the same way, let's change this rank() to be descending. http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1466 PS12, Line 1466: # Test cume_dist() desc with multiple partition exprs and multiple order by exprs : # and compare with the rewrite query Nit: Can we add a sentence before this saying this is a copy of the previous test case with the order direction flipped? http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1470 PS12, Line 1470: rank() over (partition by year, month order by id desc) as r, Nit: These statements are formulated so that the last two columns are the same. One is cum_dist() while the other is the equivalent formula. To keep this the same way, let's change this rank() to be ascending. http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1542 PS12, Line 1542: # Test ntile() desc with multiple partition exprs and multiple order by exprs and compare : # with the rewrite query Nit: Can we add a sentence before this saying this is a copy of the previous test case with the order direction flipped? http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1546 PS12, Line 1546: row_number() over (partition by year, month order by id) as rownum, Nit: These statements are formulated so that the last two columns are the same. One is ntile() while the other is the equivalent formula. To keep this the same way, let's change this row_number() to be descending. -- To view, visit http://gerrit.cloudera.org:8080/21976 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I57c69a60c63872b2964688f395b662a85698555e Gerrit-Change-Number: 21976 Gerrit-PatchSet: 12 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 26 Nov 2024 01:26:39 +0000 Gerrit-HasComments: Yes
