-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-33061][SQL] Expose inverse hyperbolic trig functions through sql.functions API #29938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This might deserve a separate JIRA ticket. Also could you might want to add a ticket id and I also suspect that omitting this in language API might be intentional, as pointed out here. |
|
Sure. I asked there why we provide wrappers around certain functions and not other and @HyukjinKwon pointed out this: spark/sql/core/src/main/scala/org/apache/spark/sql/functions.scala Lines 45 to 56 in 5af62a2
I don't insist that it necessarily applies here (though arguably, average user doesn't need hyperbolic trig functions on daily basis), just giving some context :) |
|
Thanks for the context. I agree that hyperbolic trig functions aren't everyone's cup of tea, but I think it would be better to have symmetry: cos & acos, sin & asin, tan & atan are available but we currently only have cosh, sinh & tanh without their corresponding inverse functions. |
| * @return inverse hyperbolic cosine of `e` | ||
| * | ||
| * @group math_funcs | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably have @since annotation here and for the remaining ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure whether this is something I should put in during the pull-request, or whether it gets added at a later stage.
Shall I presume that this is destined for Spark-3.1, or maybe 3.0.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be 3.1, as new functions are not added in maintenance releases.
|
There have been some discussions about which functions to add. Basically some expressions exist in SparkSQL core just for the sake of other DBMS compatibility. In other language APIs, some expressions make less sense. It's better to make simplify the call and when to add so it ended up with writing as so at #29938 (comment). Let's avoid adding functions there just for the sake of matching. How often are they used? I don't know enough about that. cc @WeichenXu123 or @srowen. Are they used commonly in data science? |
|
I imagine these are quite rarely used - more in engineering than anything I can think of in data science. Still for consistency I would not mind adding them, to make languages consistent. |
|
ok to test |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
As per my previous comments - I'm not suggesting that these inverse-hypoerboic functions will be widely used, but if I have, personally, found use-cases where an inverse-sinh is quite handy in data-science applications, for compressing the dynamic range of signed quantities, in the same way |
|
Test build #129414 has finished for PR 29938 at commit
|
I agree with that, but I wonder if consistency requires |
|
Okay, I am fine with this. |
|
Merged to master |
This patch is a small extension to change-request SPARK-28133, which added inverse hyperbolic functions to the SQL interpreter, but did not include those methods within the Scala
sql.functions._API. This patch makesacosh,asinhandatanhfunctions available through the Scala API.Unit-tests have been added to
sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala. Manual testing has been done viaspark-shell, using the following recipe:which produces the following output: