Skip to content

Commit

Permalink
[SPARK-26382][CORE] prefix comparator should handle -0.0
Browse files Browse the repository at this point in the history
## What changes were proposed in this pull request?

This is kind of a followup of apache#23239

The `UnsafeProject` will normalize special float/double values(NaN and -0.0), so the sorter doesn't have to handle it.

However, for consistency and future-proof, this PR proposes to normalize `-0.0` in the prefix comparator, so that it's same with the normal ordering. Note that prefix comparator handles NaN as well.

This is not a bug fix, but a safe guard.

## How was this patch tested?

existing tests

Closes apache#23334 from cloud-fan/sort.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
  • Loading branch information
cloud-fan authored and dongjoon-hyun committed Dec 18, 2018
1 parent 3c0bb6b commit befca98
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ public static final class DoublePrefixComparator {
* details see http://stereopsis.com/radix.html.
*/
public static long computePrefix(double value) {
// normalize -0.0 to 0.0, as they should be equal
value = value == -0.0 ? 0.0 : value;
// Java's doubleToLongBits already canonicalizes all NaN values to the smallest possible
// positive NaN, so there's nothing special we need to do for NaNs.
long bits = Double.doubleToLongBits(value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ class PrefixComparatorsSuite extends SparkFunSuite with PropertyChecks {
val nan2Prefix = PrefixComparators.DoublePrefixComparator.computePrefix(nan2)
assert(nan1Prefix === nan2Prefix)
val doubleMaxPrefix = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MaxValue)
// NaN is greater than the max double value.
assert(PrefixComparators.DOUBLE.compare(nan1Prefix, doubleMaxPrefix) === 1)
}

Expand All @@ -134,22 +135,34 @@ class PrefixComparatorsSuite extends SparkFunSuite with PropertyChecks {
assert(java.lang.Double.doubleToRawLongBits(negativeNan) < 0)
val prefix = PrefixComparators.DoublePrefixComparator.computePrefix(negativeNan)
val doubleMaxPrefix = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MaxValue)
// -NaN is greater than the max double value.
assert(PrefixComparators.DOUBLE.compare(prefix, doubleMaxPrefix) === 1)
}

test("double prefix comparator handles other special values properly") {
val nullValue = 0L
// See `SortPrefix.nullValue` for how we deal with nulls for float/double type
val smallestNullPrefix = 0L
val largestNullPrefix = -1L
val nan = PrefixComparators.DoublePrefixComparator.computePrefix(Double.NaN)
val posInf = PrefixComparators.DoublePrefixComparator.computePrefix(Double.PositiveInfinity)
val negInf = PrefixComparators.DoublePrefixComparator.computePrefix(Double.NegativeInfinity)
val minValue = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MinValue)
val maxValue = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MaxValue)
val zero = PrefixComparators.DoublePrefixComparator.computePrefix(0.0)
val minusZero = PrefixComparators.DoublePrefixComparator.computePrefix(-0.0)

// null is greater than everything including NaN, when we need to treat it as the largest value.
assert(PrefixComparators.DOUBLE.compare(largestNullPrefix, nan) === 1)
// NaN is greater than the positive infinity.
assert(PrefixComparators.DOUBLE.compare(nan, posInf) === 1)
assert(PrefixComparators.DOUBLE.compare(posInf, maxValue) === 1)
assert(PrefixComparators.DOUBLE.compare(maxValue, zero) === 1)
assert(PrefixComparators.DOUBLE.compare(zero, minValue) === 1)
assert(PrefixComparators.DOUBLE.compare(minValue, negInf) === 1)
assert(PrefixComparators.DOUBLE.compare(negInf, nullValue) === 1)
// null is smaller than everything including negative infinity, when we need to treat it as
// the smallest value.
assert(PrefixComparators.DOUBLE.compare(negInf, smallestNullPrefix) === 1)
// 0.0 should be equal to -0.0.
assert(PrefixComparators.DOUBLE.compare(zero, minusZero) === 0)
}
}

0 comments on commit befca98

Please sign in to comment.