Fix GPU degrees function does not overflow [databricks] #5996

thirtiseven · 2022-07-13T07:37:25Z

On very large numbers GpuToDegrees will not overflow and produce inf when the CPU version does. This appears to be an order of operations issues where the CPU version multiplies by 180 and then divides by pi, while we for performance reasons multiply by (180/pi).

The toDegrees in Spark is a direct call to the toDegrees in the java math library, but since JDK 9 the java implementation has changed to the same order of calculation as the current spark-rapids (from angrad * 180.0 / PI to angrad * (180.0 / PI)), So the results of spark will differ depending on the JDK version in the runtime environment.

Based on the discussion in #109, this PR keep the original implementation, add doc to explain the difference in results, and skip test_degrees when testing with jdk version 8 and lower.

Signed-off-by: thirtiseven ntlihy@gmail.com

Fixes #109.

res-life · 2022-07-13T07:48:30Z

There are 7 commits, some of them are not relevant, you can rebase -i branch-22.08 after pull upstream/branch-22.08

res-life · 2022-07-13T07:49:22Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala

-      input.getBase.mul(multiplier)
+    withResource(Scalar.fromDouble(180d)) { multiplier =>
+      withResource(input.getBase.mul(multiplier)) { dividend =>
+        dividend.div(Scalar.fromDouble(Math.PI))


Resouce leak, use withResource.

Use withResource to close Scalar.fromDouble(Math.PI)

thirtiseven · 2022-07-14T01:36:03Z

build

thirtiseven · 2022-07-14T01:37:33Z

build

thirtiseven · 2022-07-14T02:05:13Z

build

thirtiseven · 2022-07-14T08:33:09Z

build

res-life · 2022-07-14T08:45:49Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala

+    withResource(Scalar.fromDouble(180d)) { multiplier =>
+      withResource(Scalar.fromDouble(Math.PI)) { divisor =>
+        input.getBase.mul(multiplier).div(divisor)
+      }


input.getBase.mul(multiplier) produces a resource that needs to be closed.
The 180d scalar can be closed as soon as possible to release some memory. Here it's trivial but this is a good practice.

Suggests:

val tmp = withResource(Scalar.fromDouble(180d)) { multiplier => input.getBase.mul(multiplier) } // at here the 180d scalar is closed. withResource(tmp) { _ => withResource(Scalar.fromDouble(Math.PI)) { pi => tmp.div(pi) } }

ok, revised.

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven · 2022-07-14T10:56:32Z

build

revans2 · 2022-07-14T13:23:04Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/mathExpressions.scala

@@ -47,9 +47,14 @@ case class GpuAcos(child: Expression) extends CudfUnaryMathExpression("ACOS") {
 case class GpuToDegrees(child: Expression) extends GpuUnaryMathExpression("DEGREES") {

  override def doColumnar(input: GpuColumnVector): ColumnVector = {
-    withResource(Scalar.fromDouble(180d / Math.PI)) { multiplier =>
+    val tmp = withResource(Scalar.fromDouble(180d)) { multiplier =>


nit: Can we add a note so no one tries to optimize this code and make it simpler again?

Signed-off-by: thirtiseven <ntlihy@gmail.com>

…o dev

…o dev Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven · 2022-07-18T04:45:11Z

build

res-life · 2022-07-18T05:48:42Z

integration_tests/src/main/python/arithmetic_ops_test.py

@@ -611,20 +612,22 @@ def test_radians(data_gen):
    assert_gpu_and_cpu_are_equal_collect(
            lambda spark : unary_op_df(spark, data_gen).selectExpr('radians(a)'))

+def is_before_jdk_9():


Can we add some java -version samples, like:
// Allow these formats:
// 1.8.0_72-ea
// 9-ea
// 9
// 9.0.1

ok, will add some comments.

…o dev Signed-off-by: thirtiseven <ntlihy@gmail.com>

…o dev

thirtiseven · 2022-07-18T08:34:32Z

build

res-life · 2022-07-18T08:50:10Z

integration_tests/src/main/python/arithmetic_ops_test.py

+        jdk_version = jdk_version[2:]
+    dot_pos = jdk_version.find('.')
+    dash_pos = jdk_version.find('-')
+    jdk_version = jdk_version[0:dot_pos if dot_pos != -1 else dash_pos]


If jdk_version=9, will get version '', then int('') throws an exception.

res-life · 2022-07-18T08:57:25Z

integration_tests/src/main/python/arithmetic_ops_test.py

@@ -611,20 +612,30 @@ def test_radians(data_gen):
    assert_gpu_and_cpu_are_equal_collect(
            lambda spark : unary_op_df(spark, data_gen).selectExpr('radians(a)'))

+def get_java_version():
+    jdk_version = check_output(['java', '-version'], stderr=STDOUT)
+    jdk_version = jdk_version.split(b'\n')[0].split(b' ')[2].decode('utf-8')


This works well for the following version prints.

java version "1.8.0_25" Java(TM) SE Runtime Environment (build 1.8.0_25-b17) Java HotSpot(TM) 64-Bit Server VM (build 25.25-b02, mixed mode) openjdk version "1.8.0_302" OpenJDK Runtime Environment (build 1.8.0_302-bre_2021_07_21_23_11-b00) OpenJDK 64-Bit Server VM (build 25.302-b00, mixed mode)

Nit:
Seems the following code is more readable:

for line in output.splitlines(): # E.g. openjdk version "1.8.0_212" line = line.strip() if 'version' in line: ver = line.split('version', 1)[1].strip() if ver.startswith('"') and ver.endswith('"'): ver = ver[1:-1]

Thanks, revised.

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven · 2022-07-18T09:41:42Z

build

revans2

Can we add some docs to https://github.com/NVIDIA/spark-rapids/blob/branch-22.08/docs/compatibility.md? This is an odd corner case and it would be good to have users know what is happening.

revans2 · 2022-07-18T13:46:29Z

integration_tests/src/main/python/arithmetic_ops_test.py

@@ -611,20 +612,37 @@ def test_radians(data_gen):
    assert_gpu_and_cpu_are_equal_collect(
            lambda spark : unary_op_df(spark, data_gen).selectExpr('radians(a)'))

+def get_java_version():


nit: Would it be better to use the java bridge

spark._jvm.java.lang.Runtime.version()

And if that works should we move this to a common place, like with is_before_spark_320()...

I don't see a lot of others wanting to use this, but it would be nice to have it in a common place anyways.

Yes, java bridge is better, thanks. I used System.getProperty("java.version") because Runtime.version() is since JDK9.

integration_tests/src/main/python/arithmetic_ops_test.py

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven · 2022-07-19T04:26:47Z

build

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven · 2022-07-19T06:35:35Z

build

thirtiseven self-assigned this Jul 13, 2022

res-life reviewed Jul 13, 2022

View reviewed changes

res-life self-requested a review July 13, 2022 10:09

thirtiseven force-pushed the dev branch from c102f30 to 4305068 Compare July 14, 2022 01:33

thirtiseven force-pushed the dev branch from 4305068 to 6cd2f4d Compare July 14, 2022 02:03

thirtiseven changed the title ~~Fix GPU degreees function does not overflow~~ Fix GPU degreees function does not overflow [databricks] Jul 14, 2022

thirtiseven changed the title ~~Fix GPU degreees function does not overflow [databricks]~~ Fix GPU degrees function does not overflow [databricks] Jul 14, 2022

res-life reviewed Jul 14, 2022

View reviewed changes

Fix GPU degreees function does not overflow

5f10f4e

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven force-pushed the dev branch from 6cd2f4d to 5f10f4e Compare July 14, 2022 10:16

revans2 previously approved these changes Jul 14, 2022

View reviewed changes

sameerz added the bug Something isn't working label Jul 14, 2022

thirtiseven mentioned this pull request Jul 15, 2022

[BUG] GPU degreees function does not overflow #109

Closed

Fix GPU degreees function does not overflow

488c100

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven dismissed revans2’s stale review via 488c100 July 18, 2022 03:18

Haoyang Li and others added 4 commits July 18, 2022 11:24

Fix GPU degreees function does not overflow

6984505

Signed-off-by: thirtiseven <ntlihy@gmail.com>

Merge branch 'dev' of https://github.com/thirtiseven/spark-rapids int…

efbcfec

…o dev

Merge branch 'dev' of https://github.com/thirtiseven/spark-rapids int…

fe93a1a

…o dev Signed-off-by: thirtiseven <ntlihy@gmail.com>

Merge branch 'dev' of https://github.com/thirtiseven/spark-rapids int…

98e7dc0

…o dev Signed-off-by: thirtiseven <ntlihy@gmail.com>

res-life reviewed Jul 18, 2022

View reviewed changes

thirtiseven added 2 commits July 18, 2022 15:40

Merge branch 'dev' of https://github.com/thirtiseven/spark-rapids int…

d09e847

…o dev Signed-off-by: thirtiseven <ntlihy@gmail.com>

Merge branch 'dev' of https://github.com/thirtiseven/spark-rapids int…

83d3ee8

…o dev

res-life reviewed Jul 18, 2022

View reviewed changes

fix an array bound issue

5c8b886

Signed-off-by: thirtiseven <ntlihy@gmail.com>

res-life previously approved these changes Jul 18, 2022

View reviewed changes

thirtiseven marked this pull request as ready for review July 18, 2022 09:41

revans2 reviewed Jul 18, 2022

View reviewed changes

Modify test cases and add compatibility doc

2e55516

Signed-off-by: thirtiseven <ntlihy@gmail.com>

thirtiseven dismissed res-life’s stale review via 2e55516 July 19, 2022 04:15

thirtiseven added 2 commits July 19, 2022 14:18

Modified compatibility doc.

1ddfef9

Signed-off-by: thirtiseven <ntlihy@gmail.com>

Modified compatibility doc

929291d

Signed-off-by: thirtiseven <ntlihy@gmail.com>

res-life approved these changes Jul 19, 2022

View reviewed changes

revans2 approved these changes Jul 19, 2022

View reviewed changes

revans2 merged commit e945e3e into NVIDIA:branch-22.08 Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GPU degrees function does not overflow [databricks] #5996

Fix GPU degrees function does not overflow [databricks] #5996

thirtiseven commented Jul 13, 2022 •

edited

Loading

res-life commented Jul 13, 2022

res-life Jul 13, 2022

res-life Jul 14, 2022

thirtiseven Jul 14, 2022

thirtiseven commented Jul 14, 2022

thirtiseven commented Jul 14, 2022

thirtiseven commented Jul 14, 2022

thirtiseven commented Jul 14, 2022

res-life Jul 14, 2022

thirtiseven Jul 14, 2022

thirtiseven commented Jul 14, 2022

revans2 Jul 14, 2022

thirtiseven commented Jul 18, 2022

res-life Jul 18, 2022

thirtiseven Jul 18, 2022

thirtiseven commented Jul 18, 2022

res-life Jul 18, 2022 •

edited

Loading

res-life Jul 18, 2022

thirtiseven Jul 18, 2022

thirtiseven commented Jul 18, 2022

revans2 left a comment

revans2 Jul 18, 2022

thirtiseven Jul 19, 2022

thirtiseven commented Jul 19, 2022

thirtiseven commented Jul 19, 2022

Fix GPU degrees function does not overflow [databricks] #5996

Fix GPU degrees function does not overflow [databricks] #5996

Conversation

thirtiseven commented Jul 13, 2022 • edited Loading

res-life commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtiseven commented Jul 14, 2022

thirtiseven commented Jul 14, 2022

thirtiseven commented Jul 14, 2022

thirtiseven commented Jul 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtiseven commented Jul 14, 2022

Choose a reason for hiding this comment

thirtiseven commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtiseven commented Jul 18, 2022

res-life Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtiseven commented Jul 18, 2022

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtiseven commented Jul 19, 2022

thirtiseven commented Jul 19, 2022

thirtiseven commented Jul 13, 2022 •

edited

Loading

res-life Jul 18, 2022 •

edited

Loading