[SPARK-20270][SQL] na.fill should not change the values in long or integer when the default value is in double #17577

dbtsai · 2017-04-09T08:05:00Z

What changes were proposed in this pull request?

This bug was partially addressed in SPARK-18555 #15994, but the root cause isn't completely solved. This bug is pretty critical since it changes the member id in Long in our application if the member id can not be represented by Double losslessly when the member id is very big.

Here is an example how this happens, with

      Seq[(java.lang.Long, java.lang.Double)]((null, 3.14), (9123146099426677101L, null),
        (9123146560113991650L, 1.6), (null, null)).toDF("a", "b").na.fill(0.2),

the logical plan will be

== Analyzed Logical Plan ==
a: bigint, b: double
Project [cast(coalesce(cast(a#232L as double), cast(0.2 as double)) as bigint) AS a#240L, cast(coalesce(nanvl(b#233, cast(null as double)), 0.2) as double) AS b#241]
+- Project [_1#229L AS a#232L, _2#230 AS b#233]
   +- LocalRelation [_1#229L, _2#230]

Note that even the value is not null, Spark will cast the Long into Double first. Then if it's not null, Spark will cast it back to Long which results in losing precision.

The behavior should be that the original value should not be changed if it's not null, but Spark will change the value which is wrong.

With the PR, the logical plan will be

== Analyzed Logical Plan ==
a: bigint, b: double
Project [coalesce(a#232L, cast(0.2 as bigint)) AS a#240L, coalesce(nanvl(b#233, cast(null as double)), cast(0.2 as double)) AS b#241]
+- Project [_1#229L AS a#232L, _2#230 AS b#233]
   +- LocalRelation [_1#229L, _2#230]

which behaves correctly without changing the original Long values and also avoids extra cost of unnecessary casting.

How was this patch tested?

unit test added.

+cc @srowen @rxin @cloud-fan @gatorsmile

Thanks.

…lue is in double

SparkQA · 2017-04-09T09:37:24Z

Test build #75629 has finished for PR 17577 at commit cb54407.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-04-09T10:31:46Z

Test build #75630 has finished for PR 17577 at commit 42c9eb0.

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

gatorsmile · 2017-04-09T15:35:21Z

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

@@ -407,10 +407,11 @@ final class DataFrameNaFunctions private[sql](df: DataFrame) {
    val quotedColName = "`" + col.name + "`"
    val colValue = col.dataType match {
      case DoubleType | FloatType =>
-        nanvl(df.col(quotedColName), lit(null)) // nanvl only supports these types
+        // nanvl only supports these types
+        nanvl(df.col(quotedColName), lit(null).cast(col.dataType))


This is a good change, but the test case is missing. To ensure this will not be changed by others in the future, could you also add another test case? For example,

checkAnswer( Seq[(java.lang.Long, java.lang.Float)]((null, 3.14f), (9123146099426677101L, null), (9123146560113991650L, 1.6f), (null, null)).toDF("a", "b").na.fill(0.2), Row(0, 3.14f) :: Row(9123146099426677101L, 0.2f) :: Row(9123146560113991650L, 1.6f) :: Row(0, 0.2f) :: Nil )

The float one is covered by another test in fill with map, but still nice to have them explicitly to avoid being changed unexpectedly.

gatorsmile · 2017-04-09T15:36:01Z

LGTM except one comment.

SparkQA · 2017-04-09T22:37:32Z

Test build #75636 has finished for PR 17577 at commit 3fb1a66.

This patch fails from timeout after a configured wait of `250m`.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2017-04-10T00:27:00Z

Jenkins, retest this please.

viirya · 2017-04-10T02:01:06Z

LGTM

SparkQA · 2017-04-10T02:36:28Z

Test build #75638 has finished for PR 17577 at commit 3fb1a66.

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

cloud-fan · 2017-04-10T03:05:55Z

LGTM, do we still need #15994 ?

dbtsai · 2017-04-10T05:18:55Z

Merged into master.

@cloud-fan #15994 is still needed when a user wants to fill in default long value with a extremely large value into NaN. Thanks.

…teger when the default value is in double ## What changes were proposed in this pull request? This bug was partially addressed in SPARK-18555 #15994, but the root cause isn't completely solved. This bug is pretty critical since it changes the member id in Long in our application if the member id can not be represented by Double losslessly when the member id is very big. Here is an example how this happens, with ``` Seq[(java.lang.Long, java.lang.Double)]((null, 3.14), (9123146099426677101L, null), (9123146560113991650L, 1.6), (null, null)).toDF("a", "b").na.fill(0.2), ``` the logical plan will be ``` == Analyzed Logical Plan == a: bigint, b: double Project [cast(coalesce(cast(a#232L as double), cast(0.2 as double)) as bigint) AS a#240L, cast(coalesce(nanvl(b#233, cast(null as double)), 0.2) as double) AS b#241] +- Project [_1#229L AS a#232L, _2#230 AS b#233] +- LocalRelation [_1#229L, _2#230] ``` Note that even the value is not null, Spark will cast the Long into Double first. Then if it's not null, Spark will cast it back to Long which results in losing precision. The behavior should be that the original value should not be changed if it's not null, but Spark will change the value which is wrong. With the PR, the logical plan will be ``` == Analyzed Logical Plan == a: bigint, b: double Project [coalesce(a#232L, cast(0.2 as bigint)) AS a#240L, coalesce(nanvl(b#233, cast(null as double)), cast(0.2 as double)) AS b#241] +- Project [_1#229L AS a#232L, _2#230 AS b#233] +- LocalRelation [_1#229L, _2#230] ``` which behaves correctly without changing the original Long values and also avoids extra cost of unnecessary casting. ## How was this patch tested? unit test added. +cc srowen rxin cloud-fan gatorsmile Thanks. Author: DB Tsai <[email protected]> Closes #17577 from dbtsai/fixnafill. (cherry picked from commit 1a0bc41) Signed-off-by: DB Tsai <[email protected]>

…teger when the default value is in double ## What changes were proposed in this pull request? This bug was partially addressed in SPARK-18555 apache#15994, but the root cause isn't completely solved. This bug is pretty critical since it changes the member id in Long in our application if the member id can not be represented by Double losslessly when the member id is very big. Here is an example how this happens, with ``` Seq[(java.lang.Long, java.lang.Double)]((null, 3.14), (9123146099426677101L, null), (9123146560113991650L, 1.6), (null, null)).toDF("a", "b").na.fill(0.2), ``` the logical plan will be ``` == Analyzed Logical Plan == a: bigint, b: double Project [cast(coalesce(cast(a#232L as double), cast(0.2 as double)) as bigint) AS a#240L, cast(coalesce(nanvl(b#233, cast(null as double)), 0.2) as double) AS b#241] +- Project [_1#229L AS a#232L, _2#230 AS b#233] +- LocalRelation [_1#229L, _2#230] ``` Note that even the value is not null, Spark will cast the Long into Double first. Then if it's not null, Spark will cast it back to Long which results in losing precision. The behavior should be that the original value should not be changed if it's not null, but Spark will change the value which is wrong. With the PR, the logical plan will be ``` == Analyzed Logical Plan == a: bigint, b: double Project [coalesce(a#232L, cast(0.2 as bigint)) AS a#240L, coalesce(nanvl(b#233, cast(null as double)), cast(0.2 as double)) AS b#241] +- Project [_1#229L AS a#232L, _2#230 AS b#233] +- LocalRelation [_1#229L, _2#230] ``` which behaves correctly without changing the original Long values and also avoids extra cost of unnecessary casting. ## How was this patch tested? unit test added. +cc srowen rxin cloud-fan gatorsmile Thanks. Author: DB Tsai <[email protected]> Closes apache#17577 from dbtsai/fixnafill.

DB Tsai added 2 commits April 9, 2017 00:57

na.fill will change the values in long or integer when the default va…

cb54407

…lue is in double

Fix the test

42c9eb0

gatorsmile reviewed Apr 9, 2017

View reviewed changes

address feedback

3fb1a66

asfgit closed this in 1a0bc41 Apr 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-20270][SQL] na.fill should not change the values in long or integer when the default value is in double #17577

[SPARK-20270][SQL] na.fill should not change the values in long or integer when the default value is in double #17577

dbtsai commented Apr 9, 2017

SparkQA commented Apr 9, 2017

SparkQA commented Apr 9, 2017

gatorsmile Apr 9, 2017

dbtsai Apr 9, 2017

gatorsmile commented Apr 9, 2017

SparkQA commented Apr 9, 2017

dbtsai commented Apr 10, 2017

viirya commented Apr 10, 2017

SparkQA commented Apr 10, 2017

cloud-fan commented Apr 10, 2017

dbtsai commented Apr 10, 2017

[SPARK-20270][SQL] na.fill should not change the values in long or integer when the default value is in double #17577

[SPARK-20270][SQL] na.fill should not change the values in long or integer when the default value is in double #17577

Conversation

dbtsai commented Apr 9, 2017

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 9, 2017

SparkQA commented Apr 9, 2017

gatorsmile Apr 9, 2017

Choose a reason for hiding this comment

dbtsai Apr 9, 2017

Choose a reason for hiding this comment

gatorsmile commented Apr 9, 2017

SparkQA commented Apr 9, 2017

dbtsai commented Apr 10, 2017

viirya commented Apr 10, 2017

SparkQA commented Apr 10, 2017

cloud-fan commented Apr 10, 2017

dbtsai commented Apr 10, 2017