Kurt Deschler 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 10: (15 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@317 PS10, Line 317: memset(dst->ptr, 0, dst->len); Probably faster to use: *(reinterpret_cast<RegrSlopeState*>(buf)) = {}; http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@342 PS10, Line 342: memset(state, 0, sizeof(RegrSlopeState)); Probably faster to use: *(reinterpret_cast<RegrSlopeState*>(buf)) = {}; http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@386 PS10, Line 386: if (tm_src1.ToSubsecondUnixTime(UTCPTR, &val1) && Can UtcToUnixTime be used here? Even better to add a function to convert directly. http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@402 PS10, Line 402: if (tm_src1.ToSubsecondUnixTime(UTCPTR, &val1) && Can UtcToUnixTime be used here? http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@410 PS10, Line 410: RegrSlopeState* src_state = reinterpret_cast<RegrSlopeState*>(src.ptr); declare const http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@415 PS10, Line 415: int64_t nA = dst_state->count; It's often faster to not copy these member references to variables, especially on CISC processors. If they are kept, the else branch can be eliminated and nB can be moved after the return. http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@423 PS10, Line 423: double yavgB = src_state->yavg; src fields are not updated and do not need to be copied. http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@443 PS10, Line 443: RegrSlopeState* state = reinterpret_cast<RegrSlopeState*>(src.ptr); Declare as const http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@451 PS10, Line 451: double regrSlope = state->covar / state->xvar; eliminate this variable. http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@472 PS10, Line 472: -1E-8 define as constant or use an existing one http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@476 PS10, Line 476: double regrSlope = state->covar / state->xvar; Check if removing these variables improves code generation. http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@689 PS10, Line 689: CorrState* state = reinterpret_cast<CorrState*>(src.ptr); Declare const http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@729 PS10, Line 729: ctx->Free(src.ptr); Changing function to have one Free call should reduce code size http://gerrit.cloudera.org:8080/#/c/19569/10/be/src/exprs/aggregate-functions-ir.cc@751 PS10, Line 751: memset(dst->ptr, 0, dst->len); Probably faster to use: *(reinterpret_cast<CovarState*>(buf)) = {}; http://gerrit.cloudera.org:8080/#/c/19569/10/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/19569/10/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1344 PS10, Line 1344: nit: extra WS -- 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: 10 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: Thu, 18 May 2023 16:31:54 +0000 Gerrit-HasComments: Yes
