[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/18413 )
Change subject: IMPALA-11205: Implement CORR() function ...................................................................... Patch Set 10: (14 comments) > Patch Set 7: > > (16 comments) > > The patch is looking good now! http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@291 PS7, Line 291: // > nit: add a space after "//" Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@320 PS7, Line 320: st > nit: need a space after "if" Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@321 PS7, Line 321: state->prod += x * y; : ++state->count; : } : : static inline void CorrRemoveState(double x, double y, CorrState* state){ : state->sumx -= x; > We can extract these into an update method, e.g. Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@336 PS7, Line 336: if > nit: need a space after "if" Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@337 PS7, Line 337: CorrState* state = reinterpret_cast<CorrState*>(dst->ptr); : if (!isnan(src1.val) && !isnan(src2.val)) { : CorrUpdateState(src1.val, src2.val , state); : } : } : > We can extract these into a method too. Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@355 PS7, Line 355: co > nit: need a space after "if" Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@376 PS7, Line 376: ns > nit: need a space after if Done http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@401 PS7, Line 401: return DoubleVal::null(); > We should not free it here since we still use 'state' at following lines. I Approach 1 worked so implemented that http://gerrit.cloudera.org:8080/#/c/18413/7/be/src/exprs/aggregate-functions-ir.cc@408 PS7, Line 408: if > nit: add a space after "if" Done http://gerrit.cloudera.org:8080/#/c/18413/7/testdata/workloads/functional-query/queries/QueryTest/uda.test File testdata/workloads/functional-query/queries/QueryTest/uda.test: http://gerrit.cloudera.org:8080/#/c/18413/7/testdata/workloads/functional-query/queries/QueryTest/uda.test@10 PS7, Line 10: select test_count(int_col) from functional.alltypestiny; > Sorry to be back and forth, but we'd better move these tests to testdata/wo Sure, done http://gerrit.cloudera.org:8080/#/c/18413/7/testdata/workloads/functional-query/queries/QueryTest/uda.test@24 PS7, Line 24: select sum_small_decimal(c3) from functional.decimal_tiny; > Do we have test coverage on only one side is NULL? Yes, added http://gerrit.cloudera.org:8080/#/c/18413/7/testdata/workloads/functional-query/queries/QueryTest/uda.test@41 PS7, Line 41: ---- TYPES > Cool! Ack http://gerrit.cloudera.org:8080/#/c/18413/7/testdata/workloads/functional-query/queries/QueryTest/uda.test@48 PS7, Line 48: ---- RESULTS > Could you also add the 'id' column here? Done http://gerrit.cloudera.org:8080/#/c/18413/7/testdata/workloads/functional-query/queries/QueryTest/uda.test@69 PS7, Line 69: ---- TYPES > Could you add the 'year' column here? 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: 10 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 27 Apr 2022 08:41:15 +0000 Gerrit-HasComments: Yes
