Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/17023 )
Change subject: IMPALA-10473: Fix wrong analytic results on constant partition/order by exprs ...................................................................... Patch Set 2: (5 comments) Code changes look good. A few comments about testing. http://gerrit.cloudera.org:8080/#/c/17023/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test File testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test: http://gerrit.cloudera.org:8080/#/c/17023/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test@3210 PS2, Line 3210: plased nit: change to 'placed' http://gerrit.cloudera.org:8080/#/c/17023/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test@3212 PS2, Line 3212: select row_number() over (order by 'a') from functional.alltypes One other suggestion for the test since we are concerned about correctness in the presence of constants is to also add a test with expressions .. e.g order by 1+3 that exercises constant folding or CAST(5 as int). http://gerrit.cloudera.org:8080/#/c/17023/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test@3229 PS2, Line 3229: plased nit: same as above http://gerrit.cloudera.org:8080/#/c/17023/2/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test@3261 PS2, Line 3261: plased nit: same as above http://gerrit.cloudera.org:8080/#/c/17023/2/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/17023/2/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test@2200 PS2, Line 2200: select row_number() over (order by 'a'), count() over (order by 0) This query actually produces the right results without the patch because alltypestiny has only 8 rows which falls below the 100 row threshold for small query optimization and it runs as a single node plan. You could perhaps use a medium size table or reduce the threshold or (if the exact row_number value is not needed to be verified) you could do a COUNT(*) on top of the subquery with row_number() to verify row count. -- To view, visit http://gerrit.cloudera.org:8080/17023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc88a410dab984ff37e27dc635bee5f289003a2a Gerrit-Change-Number: 17023 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 04 Feb 2021 17:25:22 +0000 Gerrit-HasComments: Yes
