[SPARK-24834][CORE] use java comparison for float and double

[bavardage]

The methods in Utils, nanSafeCompareDoubles and nanSafeCompareFloats have
the same semantics as java's java.lang.Double.compare and java.lang.Float.compare
respectively.

What changes were proposed in this pull request?

Remove the two functions in Utils, and replace the call sites (only two for each function) with the corresponding java comparison function.

How was this patch tested?

Existing spark tests should cover this.

[AmplabJenkins]

Can one of the admins verify this patch?

[bavardage]

cc @JoshRosen who introduced this code originally, I think

[bavardage]

There is a way in which the nanSafeCompare* methods do differ from java built-in, but that wasn't captured in the test cases (or in the suggestive naming of the method), namely the handling of -0.0 vs 0.0.

java.lang.Double.compare(-0.0d, 0.0d) == -1
Utils.nanSafeCompareDoubles(-0.0d, 0.0d) == 0

If that behaviour is indeed required, I could abandon this PR and instead fix the test cases to capture this.

[kiszk]

I think this is required since SparkSQL does not distinguish 0.0 from -0.0. Am I correct?
cc @gatorsmile@maropu

[kiszk]

It would be good to add test cases for them since it is not covered now.

[srowen]

BTW if it becomes necessary to not change the semantics, I think the methods could at least be streamlined a bit:

if (x < y) {
-1
} else if (x > y) {
1
} else if (x == y) {
0
} else {
if (java.lang.Double.isNaN(x)) {
if (java.lang.Double.isNaN(y)) {
0
} else {
1
}
} else {
-1
}
}

More tests can't hurt, too.

[bavardage]

it does seem that spark currently does distinguish -0 and 0, at least as far as groupbys go

scala> case class Thing(x : Float)
defined class Thing
scala> val df = Seq(Thing(0.0f), Thing(-0.0f),Thing(0.0f), Thing(-0.0f),Thing(0.0f), Thing(-0.0f),Thing(0.0f), Thing(-0.0f)).toDF
2018-07-17 13:17:08 WARN ObjectStore:568 - Failed to get database global_temp, returning NoSuchObjectException
df: org.apache.spark.sql.DataFrame = [x: float]
scala> df.groupBy($"x").count
res0: org.apache.spark.sql.DataFrame = [x: float, count: bigint]
scala> res0.collect
res1: Array[org.apache.spark.sql.Row] = Array([-0.0,4], [0.0,4])

doubles are hashed via doubleToLongBitshttps://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/hash.scala#L338 which gives different bitwise representations of positive and negative 0

[srowen]

spark-sql suggests that -0 and 0 are considered the same though. SELECT -0.0 == 0.0; returns true. It's probably essential not to change behavior here, but if performance is the issue, I think the method can be optimized.

[srowen]

I think we'd have to close this due to the behavior change, but would merge an optimization of the existing behavior.

[bavardage]

yep fair - the intent I think was clarity rather than necessarily perf: it's misleading to have a method named 'nan safe' which has no special handling of nans. I'll look at opening a different PR which could increase clarity/may have minor perf benefit.