Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9346 )
Change subject: IMPALA-6230, IMPALA-6468: Fix the output type of round() and related fns ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@120 PS1, Line 120: return DoubleVal(trunc( > Do we have a test that shows the better behavior for trunc() versus the pre pow(10.0, scale.val) is likely expensive to compute and it isn't clear that the compiler will know the function is "pure". Should we instead do a table lookup and bounds check to make sure the scale parameter is sane? http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions-ir.cc@121 PS1, Line 121: v.val * pow(10.0, scale.val) + ((v.val < 0) ? -0.5 : 0.5)) / pow(10.0, scale.val)); > Should we also check for overflows here , set FunctionContext with an error Can you undo the code movement here so that the change is more clear? http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: http://gerrit.cloudera.org:8080/#/c/9346/1/be/src/exprs/math-functions.h@80 PS1, Line 80: static DoubleVal RoundUpTo(FunctionContext*, const DoubleVal&, const BigIntVal&); Why the ordering change? Also, why allow BigIntVal range here, isn't that just asking for trouble with overflow? -- To view, visit http://gerrit.cloudera.org:8080/9346 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77541678012edab70b182378b11ca8753be53f97 Gerrit-Change-Number: 9346 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Mon, 05 Mar 2018 20:16:54 +0000 Gerrit-HasComments: Yes
