[SPARK-28433][SQL][TEST] Remove hardware-dependent `0.0/0.0` and NaN comparison assertions #25186

huangtianhua · 2019-07-18T02:54:43Z

What changes were proposed in this pull request?

This PR removes a few hardware-dependent assertions which can cause a failure in aarch64.

x86_64

root@donotdel-openlab-allinone-l00242678:/home/ubuntu# uname -a
Linux donotdel-openlab-allinone-l00242678 4.4.0-154-generic #181-Ubuntu SMP Tue Jun 25 05:29:03 UTC
2019 x86_64 x86_64 x86_64 GNU/Linux

scala> import java.lang.Float.floatToRawIntBits
import java.lang.Float.floatToRawIntBits
scala> floatToRawIntBits(0.0f/0.0f)
res0: Int = -4194304
scala> floatToRawIntBits(Float.NaN)
res1: Int = 2143289344

aarch64

[root@arm-huangtianhua spark]# uname -a
Linux arm-huangtianhua 4.14.0-49.el7a.aarch64 #1 SMP Tue Apr 10 17:22:26 UTC 2018 aarch64 aarch64 aarch64 GNU/Linux

scala> import java.lang.Float.floatToRawIntBits
import java.lang.Float.floatToRawIntBits
scala> floatToRawIntBits(0.0f/0.0f)
res1: Int = 2143289344
scala> floatToRawIntBits(Float.NaN)
res2: Int = 2143289344

How was this patch tested?

Pass the Jenkins (This removes the test coverage).

yeshengm · 2019-07-18T07:28:03Z

Interesting! Could you post some refs? From my understanding, aarch64 FP ISA should fully support IEEE 754, then why there's a difference between x86_64 & aarch64?

huangtianhua · 2019-07-18T09:18:28Z

@yeshengm Hi, I post the discuss topic in JIRA issue, paste it again here:) https://users.scala-lang.org/t/the-value-of-floattorawintbits-0-0f-0-0f-is-different-on-x86-64-and-aarch64-platforms/4845

srowen · 2019-07-18T13:28:10Z

I think this change is narrowly fine. I'm actually not sure why we test the result of this JDK method directly; it's not a Spark method. @cloud-fan I think you added this?

I'm wondering if there are other usages of floatToRawIntBits, and I see only one other in QueryTest. There it's just comparing floats as bits to distinguish 0 and -0, and the varying representation of NaN probably doesn't matter. But by the same token, I think that code in QueryTest should use floatToIntBits to canonicalize NaN? maybe even have a better test to exercise NaN.

So: I think my suggestion is to delete these assertions instead.

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

cloud-fan · 2019-07-18T16:37:38Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

-    assert(doubleToRawLongBits(0.0/0.0) != doubleToRawLongBits(Double.NaN))
+    if (System.getProperty("os.arch").contains("aarch64")) {
+      // 0.0/0.0 and NaN are same value on aarch64.
+      assert(floatToRawIntBits(0.0f/0.0f) == floatToRawIntBits(Float.NaN))


We can change it to any value that is NaN but has a different binary representation than Float.NaN.

SparkQA · 2019-07-18T19:13:28Z

Test build #4824 has finished for PR 25186 at commit a0e6edd.

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

huangtianhua · 2019-07-19T04:03:25Z

So maybe delete these assertion is a good choose? The binary representation depends on hardware and seems these assertion is not related with spark function.

kiszk · 2019-07-19T04:49:47Z

Since I have no aarch64 machine, I am curious about the following statement.

seems these assertion is not related with spark function.

Did you actually see unexpected results due to this assertion?

srowen · 2019-07-19T04:58:37Z

(Yes, this assertion fails on ARM, per the thread on dev@)

maropu · 2019-07-19T05:03:01Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

-    // 0.0/0.0 and NaN are different values.
-    assert(floatToRawIntBits(0.0f/0.0f) != floatToRawIntBits(Float.NaN))
-    assert(doubleToRawLongBits(0.0/0.0) != doubleToRawLongBits(Double.NaN))
+    if (System.getProperty("os.arch").contains("aarch64")) {


Just a question; the existing tests call System.setProperty("os.arch", "xxx"), so this affects this check? e.g.,
https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/util/SizeEstimatorSuite.scala#L195

No, we ran the tests without modification of spark code on arm64, and the test failed, and also I took the tests on arm64, see:

on x86_64

root@donotdel-openlab-allinone-l00242678:/home/ubuntu# uname -a
Linux donotdel-openlab-allinone-l00242678 4.4.0-154-generic #181-Ubuntu SMP Tue Jun 25 05:29:03 UTC
2019 x86_64 x86_64 x86_64 GNU/Linux

scala> import java.lang.Float.floatToRawIntBits
import java.lang.Float.floatToRawIntBits
scala> floatToRawIntBits(0.0f/0.0f)
res0: Int = -4194304
scala> floatToRawIntBits(Float.NaN)
res1: Int = 2143289344

#on aarch64
[root@arm-huangtianhua spark]# uname -a
Linux arm-huangtianhua 4.14.0-49.el7a.aarch64 #1 SMP Tue Apr 10 17:22:26 UTC 2018 aarch64 aarch64 aarch64 GNU/Linux

scala> import java.lang.Float.floatToRawIntBits
import java.lang.Float.floatToRawIntBits
scala> floatToRawIntBits(0.0f/0.0f)
res1: Int = 2143289344
scala> floatToRawIntBits(Float.NaN)
res2: Int = 2143289344

cloud-fan · 2019-07-19T05:13:32Z

agree with @srowen that we can just remove this assertion if it depends on hardware

huangtianhua · 2019-07-19T06:21:38Z

OK, thanks all, and I will remove this assertion.

We ran unit tests of spark on aarch64 server, then found the values of floatToRawIntBits(0.0f / 0.0f) and floatToRawIntBits(Float.NaN) on aarch64 are same, after discuss with jdk-dev and scala community, we believe the value should depend on the architecture. This removes the incorrect assertions to make sure the tests fit all architectures.

cloud-fan · 2019-07-19T13:23:37Z

ok to test

SparkQA · 2019-07-19T17:48:24Z

Test build #107908 has finished for PR 25186 at commit cd5cf0c.

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

dongjoon-hyun

+1, LGTM.
I tested this PR on EC2 a1.4xlarge with OpenJDK, too.
Merged to master.

dongjoon-hyun · 2019-07-20T00:00:14Z

Thank you for your first contribution, @huangtianhua .
You are added to the Apache Spark contributor group and the following issue is assigned to you.

https://issues.apache.org/jira/browse/SPARK-28433

…comparison assertions ## What changes were proposed in this pull request? This PR removes a few hardware-dependent assertions which can cause a failure in `aarch64`. **x86_64** ``` rootdonotdel-openlab-allinone-l00242678:/home/ubuntu# uname -a Linux donotdel-openlab-allinone-l00242678 4.4.0-154-generic apache#181-Ubuntu SMP Tue Jun 25 05:29:03 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux scala> import java.lang.Float.floatToRawIntBits import java.lang.Float.floatToRawIntBits scala> floatToRawIntBits(0.0f/0.0f) res0: Int = -4194304 scala> floatToRawIntBits(Float.NaN) res1: Int = 2143289344 ``` **aarch64** ``` [rootarm-huangtianhua spark]# uname -a Linux arm-huangtianhua 4.14.0-49.el7a.aarch64 #1 SMP Tue Apr 10 17:22:26 UTC 2018 aarch64 aarch64 aarch64 GNU/Linux scala> import java.lang.Float.floatToRawIntBits import java.lang.Float.floatToRawIntBits scala> floatToRawIntBits(0.0f/0.0f) res1: Int = 2143289344 scala> floatToRawIntBits(Float.NaN) res2: Int = 2143289344 ``` ## How was this patch tested? Pass the Jenkins (This removes the test coverage). Closes apache#25186 from huangtianhua/special-test-case-for-aarch64. Authored-by: huangtianhua <huangtianhua@huawei.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

huangtianhua changed the title ~~Special scala test case for aarch64~~ [SPARK-28433] Special scala test case for aarch64 Jul 18, 2019

huangtianhua changed the title ~~[SPARK-28433] Special scala test case for aarch64~~ [SPARK-28433][SQL]Special scala test case for aarch64 Jul 18, 2019

cloud-fan reviewed Jul 18, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala Show resolved Hide resolved

cloud-fan reviewed Jul 18, 2019

View reviewed changes

maropu reviewed Jul 19, 2019

View reviewed changes

huangtianhua force-pushed the special-test-case-for-aarch64 branch from a0e6edd to cd5cf0c Compare July 19, 2019 06:35

dongjoon-hyun changed the title ~~[SPARK-28433][SQL]Special scala test case for aarch64~~ [SPARK-28433][SQL][TEST] Remove hardware-dependent 0.0/0.0 and NaN comparison assertions Jul 19, 2019

dongjoon-hyun added SQL TESTS labels Jul 19, 2019

srowen approved these changes Jul 19, 2019

View reviewed changes

dongjoon-hyun approved these changes Jul 19, 2019

View reviewed changes

dongjoon-hyun closed this in aeec6a7 Jul 19, 2019

huangtianhua deleted the special-test-case-for-aarch64 branch September 11, 2019 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-28433][SQL][TEST] Remove hardware-dependent `0.0/0.0` and NaN comparison assertions #25186

[SPARK-28433][SQL][TEST] Remove hardware-dependent `0.0/0.0` and NaN comparison assertions #25186

huangtianhua commented Jul 18, 2019 •

edited by dongjoon-hyun

Loading

yeshengm commented Jul 18, 2019

huangtianhua commented Jul 18, 2019

srowen commented Jul 18, 2019

cloud-fan Jul 18, 2019

SparkQA commented Jul 18, 2019

huangtianhua commented Jul 19, 2019

kiszk commented Jul 19, 2019

srowen commented Jul 19, 2019

maropu Jul 19, 2019

huangtianhua Jul 19, 2019

cloud-fan commented Jul 19, 2019

huangtianhua commented Jul 19, 2019

cloud-fan commented Jul 19, 2019

SparkQA commented Jul 19, 2019

dongjoon-hyun left a comment

dongjoon-hyun commented Jul 20, 2019

[SPARK-28433][SQL][TEST] Remove hardware-dependent 0.0/0.0 and NaN comparison assertions #25186

[SPARK-28433][SQL][TEST] Remove hardware-dependent 0.0/0.0 and NaN comparison assertions #25186

Conversation

huangtianhua commented Jul 18, 2019 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

How was this patch tested?

yeshengm commented Jul 18, 2019

huangtianhua commented Jul 18, 2019

srowen commented Jul 18, 2019

cloud-fan Jul 18, 2019

Choose a reason for hiding this comment

SparkQA commented Jul 18, 2019

huangtianhua commented Jul 19, 2019

kiszk commented Jul 19, 2019

srowen commented Jul 19, 2019

maropu Jul 19, 2019

Choose a reason for hiding this comment

huangtianhua Jul 19, 2019

Choose a reason for hiding this comment

on x86_64

cloud-fan commented Jul 19, 2019

huangtianhua commented Jul 19, 2019

cloud-fan commented Jul 19, 2019

SparkQA commented Jul 19, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jul 20, 2019

[SPARK-28433][SQL][TEST] Remove hardware-dependent `0.0/0.0` and NaN comparison assertions #25186

[SPARK-28433][SQL][TEST] Remove hardware-dependent `0.0/0.0` and NaN comparison assertions #25186

huangtianhua commented Jul 18, 2019 •

edited by dongjoon-hyun

Loading