Taras Bobrovytsky 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: (7 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( > pow(10.0, scale.val) is likely expensive to compute and it isn't clear that Done. According to my benchmark, this implementation is much faster (over 10x) than the old one (for cases where scale is less than 20). 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 Yes, Alex, there is a test in expr-test.cc on line 5465 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)); > Can you undo the code movement here so that the change is more clear? Done 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 Anuj, the problem here is with double arithmetic. In general, it's not possible to represent a double number precisely. Which is why it is weird to do rounding and specify the number of digits to round to in decimal. I don't think that anything can be done about this. This is exactly the reason why people should be using the decimal type. 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 Fixed the ordering. It makes sense to use bigintval because if someone passes in a smallint, it can be implicitly cast to bigint, but not the other way around. So bigint is kind of like an integer wildcard. Which overflow are you talking about? http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@266 PS1, Line 266: [['sign'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sign'], > Should we also have a DECIMAL version? Do you think we would need to have a sign() function for double, float, int, bigint, decimal, etc then? http://gerrit.cloudera.org:8080/#/c/9346/1/common/function-registry/impala_functions.py@280 PS1, Line 280: [['ceil', 'ceiling', 'dceil'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Ceil'], > Don't we also need FLOAT versions of these if our goal is to return the sam I agree, this should be addressed in a separate patch. -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Thu, 15 Mar 2018 21:51:08 +0000 Gerrit-HasComments: Yes
