pranav.lo...@cloudera.com 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 14: (7 comments) > Patch Set 14: > > (1 comment) http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@729 PS10, Line 729: // r = covar / (n) > I don't think that's what Kurt meant as this will lead to a memory leak for Done http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@420 PS13, Line 420: gA > This still copies a source field which will not be modified. If we eliminat Done http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@420 PS13, Line 420: do > Nit: too much indentation here and in the following lines. Done http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@444 PS13, Line 444: are very small, they c > We usually capitalise constants. Also, instead of "estimator", I think "thr Done http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@467 PS13, Line 467: | sta > Use the newly defined constant also here. Done http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@478 PS13, Line 478: RegrInterceptGetValue(ctx, src); > Memory leak, see comment on L729. Done http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions.h@40 PS13, Line 40: FLOATING_POINT_ERROR_T > About the name, see the comment at aggregate-functions-ir.cc L444. Done -- 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: 14 Gerrit-Owner: Anonymous Coward <pranav.lo...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <pranav.lo...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Sun, 25 Jun 2023 18:54:01 +0000 Gerrit-HasComments: Yes
