xiaoxuandev commented on code in PR #54676:
URL: https://github.com/apache/spark/pull/54676#discussion_r2935847230
##########
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:
+1, we should consider use the 2^28 threshold from fdlibm instead of
sqrt(Double.MaxValue). When x > 2^28, sqrt(x^2 ± 1) equals |x| exactly in
double precision, so the sqrt is unnecessary in that range. This aligns with
the reference implementation used by glibc, musl, and OpenJDK's StrictMath.
For acosh specifically, switching to 2^28 skips an unnecessary sqrt call for
the entire [2^28, ~1.3e154] range with no precision loss, the results are
bit-for-bit identical.
If we agree on this, I can rebase my fdlibm change on top.
--
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]
