You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by bomeng <gi...@git.apache.org> on 2016/02/05 03:29:25 UTC
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
GitHub user bomeng opened a pull request:
https://github.com/apache/spark/pull/11086
[SPARK-13145] [SQL] checkAnswer in SQL query suites should tolerate small float number error
checkAnswer will report error for small difference caused by float / double numbers.
for example, in Scala, val a = 0.1 + 0.2 will result as 0.30000000000000004, and it will result in error when comparing to 0.3
the fix will use round up and difference scale to change the values while doing the row value preparation, scale 8 / 16 should be sufficient to tolerate the errors.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bomeng/spark 13145
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/11086.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #11086
----
commit 8cbefc20b4c6f3ee3626ee7dbb6191a3ab58f187
Author: bomeng <bm...@us.ibm.com>
Date: 2016-02-05T02:22:13Z
fix SPARK-13145: approximate error for float and double type
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on the pull request:
https://github.com/apache/spark/pull/11086#issuecomment-183090608
I did not notice that method and it should work fine for double type. My question is one absTol may not be enough for all the value types, for example, float and double may use the different absTol ? But for now, I will close my PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52082988
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
```
BigDecimal(1111111111111111.1).setScale(16, BigDecimal.RoundingMode.HALF_UP).toDouble
```
This does not get round up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11086#issuecomment-180163514
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52098824
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
In your case, the difference between 2 numbers is too large to be considered as same, The 2 value should be compared in its absolute value, not relative. The float error won't be that large in the real world, and it should fail the test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52082254
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
Can you give me an example that is not working for my codes? I think using scale will trim out the small errors incurred.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52084317
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
@davies, I am still not sure in which case, 1111111111111111.1 needs to be rounded up? In my opinion, this value does not need to do so. In my test cases, compare 0.1 + 0.2 with 0.3 will fail the test cases, but it only generate a very small error, it won't be difference of 0.1. Can you give me a specific case so I can test and fix my codes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/11086#issuecomment-182986048
There is already `checkAggregatesWithTol` defined in that class, is that sufficient?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52084984
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
3.000000000000001e14 == 3.000000000000002e14
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52098701
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
This is not the purpose of this PR, if a test expect 3.000000000000001e14, but get 3.000000000000002e14, if should see the difference as float error, test passes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52097897
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
Here is my test for your case:
scala> val a = 3.000000000000001e14
a: Double = 3.000000000000001E14
scala> val b = 3.000000000000002e14
b: Double = 3.000000000000002E14
scala> val c = BigDecimal(a).setScale(16, BigDecimal.RoundingMode.HALF_UP).toDouble
c: Double = 3.000000000000001E14
scala> val d = BigDecimal(b).setScale(16, BigDecimal.RoundingMode.HALF_UP).toDouble
d: Double = 3.000000000000002E14
scala> c == d
res3: Boolean = false
So in your case, it will return false, meaning 3.000000000000001e14 != 3.000000000000002e14, as it is expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52081122
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
I think is not correct, the error is relatively small, comparing to value of it. For example, `1e15` could have a error as `1`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng closed the pull request at:
https://github.com/apache/spark/pull/11086
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52083022
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
`scale` means number of digits on the right side of dot.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/11086#issuecomment-180188439
cc @davies
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-13145] [SQL] checkAnswer in SQL query s...
Posted by bomeng <gi...@git.apache.org>.
Github user bomeng commented on a diff in the pull request:
https://github.com/apache/spark/pull/11086#discussion_r52083584
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala ---
@@ -350,6 +350,9 @@ object QueryTest {
Row.fromSeq(row.toSeq.map {
case null => null
case d: java.math.BigDecimal => BigDecimal(d)
+ // Round up the values to avoid approximate error
+ case f : Float => BigDecimal(f).setScale(8, BigDecimal.RoundingMode.HALF_UP).toFloat
--- End diff --
I have also tested this case:
val a = 1e15
val b = 1e15 - 1
val c = BigDecimal(a).setScale(16, BigDecimal.RoundingMode.HALF_UP).toDouble
// c: Double = 1.0E15
val d = BigDecimal(b).setScale(16, BigDecimal.RoundingMode.HALF_UP).toDouble
// d: Double = 9.99999999999999E14
c == d
// res6: Boolean = false
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org