[email protected] 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 22: (21 comments) > Patch Set 20: > > (19 comments) http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@301 PS20, Line 301: int64_t count; // number of elements > Change all occurrences of long to int64_t Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@341 PS20, Line 341: ) > nit: remove the space before ")" Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@342 PS20, Line 342: else { : --state->count; : // mx_(n - 1) = mx_n - (x_n - mx_n) / (n - 1) : state->xavg -= deltaX / state->count; : // my_(n - 1) = my_n - (y_n - my_n) / (n - 1) : state->yavg -= delt > nit: these can be simplified by memset(), just like what we do at line 313. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@348 PS20, Line 348: // c_(n - 1) = c_n - (x_n - mx_n) * (y_n - my_(n -1)) : st > nit: put "}" and "else" at the same line, i.e. "} else {" Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@355 PS20, Line 355: > This comment seems incorrect. 'deltaX' is actually (x_n - mx_n). 'y - state Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@375 PS20, Line 375: > This is not clear to me. Do you mean 'count'? In functions which don't have finalize() function, we need to check explicitly if num_removes() >= num_updates(), but here since we have finalize() function, we don't need to explicitly check the number of update() and remove() calls. http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@378 PS20, Line 378: void AggregateFunctions::TimestampCorrUpdate(FunctionContext* ctx, > Change all occurrences of NULL to nullptr Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@393 PS20, Line 393: // Remove doesn't need to explicitly > nit: please add brackets {} for multi-line if-statement. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@398 PS20, Line 398: lue::FromTimestampVal(src1); > Could you explain more? Why do we need to check the number of Update/Remove In functions which don't have finalize() function, we need to check explicitly if num_removes() >= num_updates(), but here since we have finalize() function, we don't need to explicitly check the number of update() and remove() calls. http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@407 PS20, Line 407: void AggregateFunctions::CorrMerge(Func > nit: please add brackets {} for multi-line if-statement. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@451 PS20, Line 451: e->yvar >= -1E-8); > We should check whether 'state->xvar * state->yvar' become negative before Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@452 PS20, Line 452: if (state->count == 0 || state->count == 1 || state->xvar <= 0.0 || state->yvar <= 0.0) : return DoubleVal::null(); : double r = sqrt(state->xvar * state->yvar); > move this to the begining of this method, and split it into two DCHECKs so Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@467 PS20, Line 467: ctx->Free(src.ptr); : return r; > This is a use-after-free pattern. We should not free the resource if we sti Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@507 PS20, Line 507: ) > nit: remove the space before ")". Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@508 PS20, Line 508: else { : --state->count; : // my_(n - 1) = my_n - (y_n - my_n) / (n - 1) : state->yavg -= (y - > nit: use memset instead. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@512 PS20, Line 512: // c_(n - 1) = c_n - (x_n - mx_(n - 1)) * (y_n - my_n) : stat > nit: put them into one line, i.e. "} else {" Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@535 PS20, Line 535: > Need explanation here too In functions which don't have finalize() function, we need to check explicitly if num_removes() >= num_updates(), but here since we have finalize() function, we don't need to explicitly check the number of update() and remove() calls. http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@553 PS20, Line 553: void AggregateFunctions::TimestampCovarR > nit: add brackets {} for multi-line if-statement. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@567 PS20, Line 567: } > nit: add brackets {} for multi-line if-statement. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@625 PS20, Line 625: ctx->Free(src.ptr); : return r; > This is a use-after-free pattern too. Please fix it. Done http://gerrit.cloudera.org:8080/#/c/18413/20/be/src/exprs/aggregate-functions-ir.cc@636 PS20, Line 636: DoubleVal r = CovarPopulationGetValue(ctx, src); : ctx->Free(src.ptr); > This is a use-after-free pattern too. Please fix it. Done -- 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: 22 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jian Zhang <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 08 Jun 2022 18:24:50 +0000 Gerrit-HasComments: Yes
