Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19569 )
Change subject: IMPALA-11957: Implement Regression functions: regr_slope(), regr_intercept() and regr_r2() ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/19569/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19569/9//COMMIT_MSG@18 PS9, Line 18: regr_r2() takes two arguments of numeric type and returns the coefficient This line is still too long (73 vs. 72). http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@702 PS7, Line 702: > To cross check the results, I did run the queries on postgreSQL as well, an I'll try to reproduce the situation myself and see what Hive does. http://gerrit.cloudera.org:8080/#/c/19569/9/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/19569/9/be/src/exprs/aggregate-functions-ir.cc@683 PS9, Line 683: ) There's no matching opening parenthesis. I think the opening paren should come before the "and var_pop(x)". http://gerrit.cloudera.org:8080/#/c/19569/9/be/src/exprs/aggregate-functions-ir.cc@685 PS9, Line 685: // (var_pop(y) != 0 and var_pop(x) != 0) Fits on the previous line, after the if. http://gerrit.cloudera.org:8080/#/c/19569/9/be/src/exprs/aggregate-functions-ir.cc@692 PS9, Line 692: // dependent_var and independent_var become negative in certain cases due to floating : // point rounding error. : // Since these values are very small, they can be ignored and rounded to 0. This could come after assigning 'dependent_var' and 'independent_var', before the DCHECKs. http://gerrit.cloudera.org:8080/#/c/19569/9/be/src/exprs/aggregate-functions-ir.cc@712 PS9, Line 712: dependent_var * independent_var Shouldn't we also divide these by state->count? In that case we could include the division from the beginning, i.e. the definition of 'dependent_var' and 'independent_var'. http://gerrit.cloudera.org:8080/#/c/19569/9/be/src/exprs/aggregate-functions-ir.cc@714 PS9, Line 714: are Nit: is. -- To view, visit http://gerrit.cloudera.org:8080/19569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab6bd84ae3e0c02ec924c30183308123b951caa3 Gerrit-Change-Number: 19569 Gerrit-PatchSet: 9 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 12 May 2023 15:15:49 +0000 Gerrit-HasComments: Yes
