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 13: (7 comments) 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: // population covariance between two columns of numeric types respectively using > Done I don't think that's what Kurt meant as this will lead to a memory leak for NULLs - we allocate a buffer in RegrSlopeInit() but don't free it now. I think Kurt meant something like this: { DoubleVal r = src.is_null ? DoubleVal::null() : Regr_r2GetValue(ctx, src); ctx->Free(src.ptr); return r; } 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: nB This still copies a source field which will not be modified. If we eliminate the copies of the source variables, this should be eliminated too. http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@420 PS13, Line 420: Nit: too much indentation here and in the following lines. http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@444 PS13, Line 444: FloatingErrorEstimator We usually capitalise constants. Also, instead of "estimator", I think "threshold" would be better, so for example: FLOATING_POINT_ERROR_THRESHOLD or FP_ERROR_THRESHOLD. http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@467 PS13, Line 467: -1E-8 Use the newly defined constant also here. http://gerrit.cloudera.org:8080/#/c/19569/13/be/src/exprs/aggregate-functions-ir.cc@478 PS13, Line 478: if (UNLIKELY(src.is_null)) { Memory leak, see comment on L729. 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: FloatingErrorEstimator About the name, see the comment at aggregate-functions-ir.cc L444. -- 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: 13 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: Tue, 20 Jun 2023 10:05:45 +0000 Gerrit-HasComments: Yes
