Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18413 )
Change subject: IMPALA-11205: Implement Statistical functions : CORR(), COVAR_SAMP() and COVAR_POP() ...................................................................... Patch Set 12: (12 comments) Thanks for adding the other two stats functions! http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc@301 PS12, Line 301: // nit: add a space http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc@309 PS12, Line 309: // Sum(x) squared : double sum_squaredx; : // Sum(y) squared : double sum_squaredy; These two fields are not used in covar_samp() and covar_pop(). Can we have a separate State for covar_samp() and covar_pop()? http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc@349 PS12, Line 349: ,src2.val , nit: please fix the space http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc@362 PS12, Line 362: ,src2.val , nit: fix the space http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc@376 PS12, Line 376: ,val2, nit: add spaces http://gerrit.cloudera.org:8080/#/c/18413/12/be/src/exprs/aggregate-functions-ir.cc@392 PS12, Line 392: ,val2, nit: add spaces http://gerrit.cloudera.org:8080/#/c/18413/12/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java: http://gerrit.cloudera.org:8080/#/c/18413/12/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1347 PS12, Line 1347: // nit: add one space http://gerrit.cloudera.org:8080/#/c/18413/12/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1372 PS12, Line 1372: // nit: add one space http://gerrit.cloudera.org:8080/#/c/18413/12/testdata/workloads/functional-query/queries/QueryTest/aggregation.test File testdata/workloads/functional-query/queries/QueryTest/aggregation.test: http://gerrit.cloudera.org:8080/#/c/18413/12/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1526 PS12, Line 1526: select corr(s_number_employees,s_floor_space) over (partition by s_city) from tpcds.store limit 10; nit: could you add s_store_sk as the first column? Also we don't need "limit 10" since this table only has 12 rows. http://gerrit.cloudera.org:8080/#/c/18413/12/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1619 PS12, Line 1619: /1E+10 Could you leave a commet for this? http://gerrit.cloudera.org:8080/#/c/18413/12/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1683 PS12, Line 1683: select covar_samp(s_number_employees, s_floor_space) over (partition by s_city) from tpcds.store limit 10; nit: add s_store_sk column and remove the limit clause http://gerrit.cloudera.org:8080/#/c/18413/12/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@1700 PS12, Line 1700: select covar_pop(s_number_employees, s_floor_space) over (partition by s_city) from tpcds.store limit 10; nit: add s_store_sk column and remove the limit clause -- To view, visit http://gerrit.cloudera.org:8080/18413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32ad627c953ba24d9cde2d5549bdd0d27a9c0d06 Gerrit-Change-Number: 18413 Gerrit-PatchSet: 12 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 12 May 2022 07:48:45 +0000 Gerrit-HasComments: Yes
