[SPARK-28133][SQL] Add acosh/asinh/atanh functions to SQL #25041

Tonix517 · 2019-07-03T04:38:11Z

What changes were proposed in this pull request?

Adding support to hyperbolic functions like asinh\acosh\atanh in spark SQL.
Feature parity: https://www.postgresql.org/docs/12/functions-math.html#FUNCTIONS-MATH-HYP-TABLE

The followings are the diffence from PostgreSQL.

spark-sql> SELECT acosh(0);     (PostgreSQL returns `ERROR:  input is out of range`)
NaN

spark-sql> SELECT atanh(2);     (PostgreSQL returns `ERROR:  input is out of range`)
NaN

Teradata has similar behavior as PostgreSQL with out of range input float values - It outputs Invalid Input: numeric value within range only.

These newly added asinh/acosh/atanh handles special input(NaN, +-Infinity) in the same way as existing cos/sin/tan/acos/asin/atan in spark. For which input value range is not (-∞, ∞)):
out of range float values: Spark returns NaN and PostgreSQL shows input is out of range
NaN: Spark returns NaN, PostgreSQL also returns NaN
Infinity: Spark return NaN, PostgreSQL shows input is out of range

How was this patch tested?

spark.sql("select asinh(xx)")
spark.sql("select acosh(xx)")
spark.sql("select atanh(xx)")

./build/sbt "testOnly org.apache.spark.sql.MathFunctionsSuite"
./build/sbt "testOnly org.apache.spark.sql.catalyst.expressions.MathExpressionsSuite"

HyukjinKwon · 2019-07-03T04:58:03Z

ok to test

dongjoon-hyun · 2019-07-03T05:05:16Z

cc @gatorsmile

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

dongjoon-hyun · 2019-07-03T05:20:36Z

Welcome, @Tonix517 . Thank you for your first contribution.

SparkQA · 2019-07-03T05:38:53Z

Test build #107156 has finished for PR 25041 at commit febdbef.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-03T07:05:02Z

Test build #107152 has finished for PR 25041 at commit 89d708e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-03T07:05:02Z

Test build #107159 has finished for PR 25041 at commit 0e72b3d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

Tonix517 · 2019-07-03T15:27:17Z

Anyway to rerun the test? It failed with a SIGKILL as below:

[error] running /home/jenkins/workspace/SparkPullRequestBuilder@3/build/sbt -Phadoop-2.7 -Phive-thriftserver -Phive -Dtest.exclude.tags=org.apache.spark.tags.ExtendedHiveTest,org.apache.spark.tags.ExtendedYarnTest hive-thriftserver/test avro/test mllib/test hive/test repl/test catalyst/test sql/test sql-kafka-0-10/test examples/test ; process was terminated by signal 9

mgaido91 · 2019-07-03T15:30:12Z

retest this please

SparkQA · 2019-07-03T18:55:54Z

Test build #107188 has finished for PR 25041 at commit 0e72b3d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

Tonix517 · 2019-07-03T20:39:16Z

retest this please

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathExpressionsSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathExpressionsSuite.scala

SparkQA · 2019-07-12T20:09:05Z

Test build #107607 has finished for PR 25041 at commit bd4e7ff.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

SparkQA · 2019-07-13T00:41:47Z

Test build #107617 has finished for PR 25041 at commit 10c30b0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-13T01:09:37Z

Test build #107618 has finished for PR 25041 at commit b2db2f5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-07-13T19:04:41Z

@Tonix517 . I'm still hitting the following weird codegen result. I really meant it to see the generate code by your eyes.

/* 035 */         project_value_0 = project_value_0 = inputadapter_value_0 == Double.NEGATIVE_INFINITY ?
Double.NEGATIVE_INFINITY :
java.lang.Math.log(inputadapter_value_0 + java.lang.Math.sqrt(inputadapter_value_0 * inputadapter_value_0 + 1.0))
;;

dongjoon-hyun · 2019-07-13T19:07:16Z

As @mgaido91 's suggestion, you had better rewrite with if ... else statement.
And, again, please check the generated code manually. The generate code is also a part of your PR. The best result is the Java code looks like the code you write by your hands.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

Tonix517 · 2019-07-14T03:39:21Z

Thanks @dongjoon-hyun . @mgaido91 's suggestion is correct. With his suggestion, now the generated code will be:

project_value_0 = inputadapter_value_0 == Double.NEGATIVE_INFINITY ? Double.NEGATIVE_INFINITY : java.lang.Math.log(inputadapter_value_0 + java.lang.Math.sqrt(inputadapter_value_0 * inputadapter_value_0 + 1.0));

which is in much better shape than previous implementations. and apparently doCodeGen should return an expression not a statement.

As @mgaido91 's suggestion, you had better rewrite with if ... else statement.
And, again, please check the generated code manually. The generate code is also a part of your PR. The best result is the Java code looks like the code you write by your hands.

dongjoon-hyun · 2019-07-14T06:07:17Z

@Tonix517 . doGenCode can return statements. What he mentioned was defineCodeGen.

and apparently doCodeGen should return an expression not a statement.

Anyway, yes. The newly generate code is right now. Thanks.

dongjoon-hyun · 2019-07-14T06:20:18Z

I'm doing the final review. If there is no issue, I can merge this tonight, @Tonix517 .

SparkQA · 2019-07-14T06:48:01Z

Test build #107635 has finished for PR 25041 at commit cd97401.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2019-07-14T07:06:33Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+    case Double.NegativeInfinity => Double.NegativeInfinity
+    case _ => math.log(x + math.sqrt(x * x + 1.0)) }, "ASINH") {
+  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+    defineCodeGen(ctx, ev, c =>


as I mentioned earlier, I would prefer rewriting this as an if. There may be stage cases where $c contains a cast or something similar which may cause issues (I has similar problem in another PR, Janino is not perfect with this syntax in the version we're using). So it think it would be safer to rewrite as an if

I also agreed to that. Could you give us the pointer to that PR(or some example)? Actually, I tried to reproduce that kind of issue within the scope of this PR. Until now, I didn't succeed. Maybe, we had better a test case for that to make it sure.

The PR was this one: #24636. The bug opened there for janino cannot happen here, but I remember I also had issues in that PR in a case when there was a cast added to the terms.

Thanks, @mgaido91 !

dongjoon-hyun · 2019-07-14T16:53:25Z

@Tonix517 and @mgaido91 .
To sum up, the last thing we need to check was janino-compiler/janino#90 which is reported by Apache Spark community at #24636.

In the old PR #24636, we also used cond ? true : false expression.

s"Object $argTerm = ${eval.isNull} ? null : $convertersTerm[$i].apply(${eval.value});"

It seems that we are not in that case, but let me check that again today.

dongjoon-hyun · 2019-07-14T17:10:59Z

For the second case, a cast added to the terms,

The bug opened there for janino cannot happen here, but I remember I also had issues in that PR in a case when there was a cast added to the terms.

I tested some, but like line 37, it's declared already double project_value_1 = -1.0; before reaching this math function.

/* 037 */       double project_value_1 = -1.0;
/* 038 */       if (!project_isNull_2) {
/* 039 */         try {
/* 040 */           project_value_1 = Double.valueOf(project_value_2.toString());
/* 041 */         } catch (java.lang.NumberFormatException e) {
/* 042 */           project_isNull_1 = true;
/* 043 */         }
/* 044 */       }
/* 045 */       boolean project_isNull_0 = project_isNull_1;
/* 046 */       double project_value_0 = -1.0;
/* 047 */
/* 048 */       if (!project_isNull_1) {
/* 049 */         project_value_0 = project_value_1 == Double.NEGATIVE_INFINITY ? Double.NEGATIVE_INFINITY : java.lang.Math.log(project_value_1 + java.lang.Math.sqrt(project_value_1 * project_value_1 + 1.0));
/* 050 */       }

dongjoon-hyun · 2019-07-14T17:16:00Z

@mgaido91 . Do you have a counter-example? For me, this function avoids both two corner cases.

mgaido91 · 2019-07-14T18:42:02Z

@dongjoon-hyun , thanks for checking. I agree with you that case 1, which is the bug reported to janino cannot happen here, so it is not an issue here. I am more worried about case 2. Because case 2 depends on what is the input of the function and I think it is hard to predict.

Do you have a counter-example?

No I don't have honestly. So I'd prefer the if syntax to be on the safe side. We can also go ahead like this if you think it is fine and eventually change it later if an issue shows up.

dongjoon-hyun · 2019-07-15T03:39:10Z

Thank you, @mgaido91 .

Yes, I tried in several use cases including CAST/CTAS/INSERT/WHERE for the second case. The argument of that function is stored into double primitive variables like double project_value_1 or double filter_value_2 already. It turns out pure double operations.

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you so much for your contribution and patience, @Tonix517.
Also, thank you so much, @maropu and @mgaido91 ! Without your helps, this PR cannot be merged.

Tonix517 · 2019-07-15T03:48:51Z

@dongjoon-hyun @mgaido91 Thank you very much for all your guidance and patience. Learned a lot :)

HyukjinKwon · 2019-07-15T03:49:12Z

+1 looks fine to me too.

mgaido91 · 2019-07-15T09:10:18Z

Thank you @dongjoon-hyun and thank you @Tonix517 for your contribution!

## What changes were proposed in this pull request? Adding support to hyperbolic functions like asinh\acosh\atanh in spark SQL. Feature parity: https://www.postgresql.org/docs/12/functions-math.html#FUNCTIONS-MATH-HYP-TABLE The followings are the diffence from PostgreSQL. ``` spark-sql> SELECT acosh(0); (PostgreSQL returns `ERROR: input is out of range`) NaN spark-sql> SELECT atanh(2); (PostgreSQL returns `ERROR: input is out of range`) NaN ``` Teradata has similar behavior as PostgreSQL with out of range input float values - It outputs **Invalid Input: numeric value within range only.** These newly added asinh/acosh/atanh handles special input(NaN, +-Infinity) in the same way as existing cos/sin/tan/acos/asin/atan in spark. For which input value range is not (-∞, ∞)): out of range float values: Spark returns NaN and PostgreSQL shows input is out of range NaN: Spark returns NaN, PostgreSQL also returns NaN Infinity: Spark return NaN, PostgreSQL shows input is out of range ## How was this patch tested? ``` spark.sql("select asinh(xx)") spark.sql("select acosh(xx)") spark.sql("select atanh(xx)") ./build/sbt "testOnly org.apache.spark.sql.MathFunctionsSuite" ./build/sbt "testOnly org.apache.spark.sql.catalyst.expressions.MathExpressionsSuite" ``` Closes apache#25041 from Tonix517/SPARK-28133. Authored-by: Tony Zhang <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Tony Zhang added 4 commits July 2, 2019 21:29

[SPARK-28133] Adding inverse hyperbolic functions in SQL

605df14

[SPARK-28133] Adding inverse hyperbolic functions in SQL

657c053

[SPARK-28133] Adding inverse hyperbolic functions in SQL

2580f50

[SPARK-28133] Adding inverse hyperbolic functions in SQL

89d708e

dongjoon-hyun changed the title ~~[Spark 28133] Add hyperbolic functions support in SQL~~ [SPARK-28133][SQL] Add hyperbolic functions support in SQL Jul 3, 2019

dongjoon-hyun added the SQL label Jul 3, 2019

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

dongjoon-hyun changed the title ~~[SPARK-28133][SQL] Add hyperbolic functions support in SQL~~ [SPARK-28133][SQL] Add acosh/asinh/atanh functions Jul 3, 2019

dongjoon-hyun changed the title ~~[SPARK-28133][SQL] Add acosh/asinh/atanh functions~~ [SPARK-28133][SQL] Add acosh/asinh/atanh functions Jul 3, 2019

Tony Zhang added 2 commits July 2, 2019 22:22

[SPARK-28133] Adding inverse hyperbolic functions in SQL

a761f1f

[SPARK-28133] Adding inverse hyperbolic functions in SQL

febdbef

[SPARK-28133] Adding inverse hyperbolic functions in SQL

0e72b3d

dongjoon-hyun changed the title ~~[SPARK-28133][SQL] Add acosh/asinh/atanh functions~~ [SPARK-28133][SQL] Add acosh/asinh/atanh functions to SQL Jul 3, 2019

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated Show resolved Hide resolved

[SPARK-28133] Adding inverse hyperbolic functions in SQL

a495dde

dongjoon-hyun reviewed Jul 3, 2019

View reviewed changes

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathExpressionsSuite.scala Outdated Show resolved Hide resolved

mgaido91 reviewed Jul 12, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala Outdated Show resolved Hide resolved

...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathExpressionsSuite.scala Show resolved Hide resolved

[SPARK-28133][SQL] Adding inverse hyperbolic functions in SQL

bd4e7ff

Tony Zhang added 2 commits July 12, 2019 14:18

[SPARK-28133][SQL] Adding inverse hyperbolic functions in SQL

10c30b0

[SPARK-28133][SQL] Adding inverse hyperbolic functions in SQL

b2db2f5

dongjoon-hyun reviewed Jul 12, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala Outdated Show resolved Hide resolved

mgaido91 suggested changes Jul 13, 2019

View reviewed changes

[SPARK-28133][SQL] Adding inverse hyperbolic functions in SQL

cd97401

mgaido91 reviewed Jul 14, 2019

View reviewed changes

dongjoon-hyun approved these changes Jul 15, 2019

View reviewed changes

dongjoon-hyun closed this in a2f71a8 Jul 15, 2019

Tonix517 deleted the SPARK-28133 branch July 27, 2019 05:25

[SPARK-28133][SQL] Add acosh/asinh/atanh functions to SQL #25041

[SPARK-28133][SQL] Add acosh/asinh/atanh functions to SQL #25041

Uh oh!

Conversation

Tonix517 commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Jul 3, 2019

Uh oh!

dongjoon-hyun commented Jul 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

Tonix517 commented Jul 3, 2019

Uh oh!

mgaido91 commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tonix517 commented Jul 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jul 12, 2019

Uh oh!

Uh oh!

SparkQA commented Jul 13, 2019

Uh oh!

SparkQA commented Jul 13, 2019

Uh oh!

dongjoon-hyun commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tonix517 commented Jul 14, 2019

Uh oh!

dongjoon-hyun commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 14, 2019

Uh oh!

SparkQA commented Jul 14, 2019

Uh oh!

mgaido91 Jul 14, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 14, 2019

Choose a reason for hiding this comment

Uh oh!

mgaido91 Jul 14, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 14, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 14, 2019

Uh oh!

dongjoon-hyun commented Jul 14, 2019

Uh oh!

dongjoon-hyun commented Jul 14, 2019

Tonix517 commented Jul 3, 2019 •

edited

Loading

dongjoon-hyun commented Jul 13, 2019 •

edited

Loading

dongjoon-hyun commented Jul 13, 2019 •

edited

Loading

dongjoon-hyun commented Jul 14, 2019 •

edited

Loading