[email protected] 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 8: (11 comments) > Patch Set 7: > > (10 comments) http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@299 PS7, Line 299: > line too long (151 > 90) Since its a hyperlink, it should be in the same line for easy access. http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@678 PS7, Line 678: // CorrState is reused for implementing regr_r2. > In source files the max line width is 90. Many lines here are shorter than Noted, done! http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@679 PS7, Line 679: ar > Nit: no need for this 'is'. Done http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@684 PS7, Line 684: // power(corr(y, x),2) if > I think using 'dependent' and 'independent' here in the doc comment is over Done http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@685 PS7, Line 685: p > You left out var_pop. Done http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@688 PS7, Line 688: DoubleVal AggregateFunctions::Regr_r2GetValue(FunctionContext* ctx, > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@698 PS7, Line 698: // CorrUpdate(), which we use to produce the intermediate values, has the opposite > We should add a comment describing why we assign 'state->xvar' to 'dependen Done http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@702 PS7, Line 702: > This still doesn't seem right. If 'dependent_var' is less than zero, then w To cross check the results, I did run the queries on postgreSQL as well, and this is how its functioning there, we can discuss more about it offline. http://gerrit.cloudera.org:8080/#/c/19569/7/be/src/exprs/aggregate-functions-ir.cc@708 PS7, Line 708: lse if (dependent_var == 0.0) { : return 1; > This is unnecessary here, the following lines contain this information. Done http://gerrit.cloudera.org:8080/#/c/19569/7/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/7/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@1344 PS7, Line 1344: > Nit: trailing whitespace, should be removed. There's usually a line break before the definitions of new functions. Please let me know if you mean something else. http://gerrit.cloudera.org:8080/#/c/19569/7/testdata/workloads/functional-query/queries/QueryTest/aggregation.test File testdata/workloads/functional-query/queries/QueryTest/aggregation.test: http://gerrit.cloudera.org:8080/#/c/19569/7/testdata/workloads/functional-query/queries/QueryTest/aggregation.test@2398 PS7, Line 2398: (state->xvar <= 0.0), without which the be > This is not accurate, we shouldn't handle these cases as the same. See comm Done, let me know if you think that is okay. -- 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: 8 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: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 09 May 2023 17:37:44 +0000 Gerrit-HasComments: Yes
