Steve Carlin 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 13: (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: * = Rank/Count : * where, : * Rank = rank() over([partition by clause] order by clause) : * Count = count() over([partition by clause]) > This seems to be the same as Rank / Count where Ok, I made this change and also changed the formula in the test file and it seemed to work fine. I'm not aware of edge cases, but if you think it's safe, I'm all for changing it. 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: RelDataType bigintType = > Nit: descendingSet is unused Done 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: over.getOperands(), over.getWindow().partitionKeys, over.getWindow().orderKeys, : over.getWindow().getLowerBound(), over.getWindow().getUpperBound(), : over.getWindow().isRows(), true, false, over.isDistinct(), over.ignoreNulls()); : RexNode countNode = rexBuilder.makeOver(bigintType, : ImpalaCustomOperatorTable.COUNT, over.getOperands(), > Nit: descendingCollation and allDescendingCollation are unused Done 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: > I don't think we want to reverse this list. If we have (A asc, B asc, C asc No longer needed if we don't flip the order 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 previou Done http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1394 PS12, Line 1394: t.pr, (t.r-1)/(t.c-1) > Nit: These statements are formulated so that the last two columns are the s Done http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1466 PS12, Line 1466: ---- QUERY : # Test cume_dist() desc with multipl > Nit: Can we add a sentence before this saying this is a copy of the previou Done http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1470 PS12, Line 1470: select t.year, t.month, t.id, t.cd, t.r/t.c > Nit: These statements are formulated so that the last two columns are the s Done http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1542 PS12, Line 1542: ==== : ---- QUERY > Nit: Can we add a sentence before this saying this is a copy of the previou Done http://gerrit.cloudera.org:8080/#/c/21976/12/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@1546 PS12, Line 1546: # This is a copy of the previous percent_rank test with the order f > Nit: These statements are formulated so that the last two columns are the s Done -- 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: 13 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: Wed, 27 Nov 2024 15:10:07 +0000 Gerrit-HasComments: Yes
