xiaoxuandev commented on code in PR #54676:
URL: https://github.com/apache/spark/pull/54676#discussion_r2943422255
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala:
##########
@@ -418,10 +418,28 @@ case class Cosh(child: Expression) extends
UnaryMathExpression(math.cosh, "COSH"
since = "3.0.0",
group = "math_funcs")
case class Acosh(child: Expression)
- extends UnaryMathExpression((x: Double) => StrictMath.log(x + math.sqrt(x *
x - 1.0)), "ACOSH") {
+ extends UnaryMathExpression((x: Double) => x match {
+ // in case of large values, the square would lead to Infinity; also, - 1
would be ignored due
+ // to numeric precision. So log(x + sqrt(x * x - 1)) becomes log(2x) =
log(2) + log(x) for
+ // positive values.
+ case x if x >= Math.sqrt(Double.MaxValue) =>
Review Comment:
Thank you @mgaido91 ! Actually I was wrong about no precision loss for
keeping the log(x + sqrt(x²±1)) formula for most inputs. During testing this
can lose precision due to catastrophic cancellation, specifically for acosh
when 1 < x < 2 and asinh when |x| < 2.
I've updated this PR(#54677) to fully align with the fdlibm algorithm
(e_acosh.c / s_asinh.c), matching the OpenJDK implementation. The key additions
are the middle-range formula (log(2x ± 1/(…)) for 2 < x < 2^28) and log1p for
values near 1/0. Now the results are closer to the exact mathematical values.
@peter-toth @sarutak Would appreciate another look when you guys get a
chance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
