Skip to content

Commit 6a1dc22

Browse files
cloud-fanJackey Lee
authored andcommitted
[SPARK-26382][CORE] prefix comparator should handle -0.0
## 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 <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
1 parent f90cfe6 commit 6a1dc22

File tree

2 files changed

+17
-2
lines changed

2 files changed

+17
-2
lines changed

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/PrefixComparators.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ public static final class DoublePrefixComparator {
6969
* details see http://stereopsis.com/radix.html.
7070
*/
7171
public static long computePrefix(double value) {
72+
// normalize -0.0 to 0.0, as they should be equal
73+
value = value == -0.0 ? 0.0 : value;
7274
// Java's doubleToLongBits already canonicalizes all NaN values to the smallest possible
7375
// positive NaN, so there's nothing special we need to do for NaNs.
7476
long bits = Double.doubleToLongBits(value);

core/src/test/scala/org/apache/spark/util/collection/unsafe/sort/PrefixComparatorsSuite.scala

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ class PrefixComparatorsSuite extends SparkFunSuite with PropertyChecks {
125125
val nan2Prefix = PrefixComparators.DoublePrefixComparator.computePrefix(nan2)
126126
assert(nan1Prefix === nan2Prefix)
127127
val doubleMaxPrefix = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MaxValue)
128+
// NaN is greater than the max double value.
128129
assert(PrefixComparators.DOUBLE.compare(nan1Prefix, doubleMaxPrefix) === 1)
129130
}
130131

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

140142
test("double prefix comparator handles other special values properly") {
141-
val nullValue = 0L
143+
// See `SortPrefix.nullValue` for how we deal with nulls for float/double type
144+
val smallestNullPrefix = 0L
145+
val largestNullPrefix = -1L
142146
val nan = PrefixComparators.DoublePrefixComparator.computePrefix(Double.NaN)
143147
val posInf = PrefixComparators.DoublePrefixComparator.computePrefix(Double.PositiveInfinity)
144148
val negInf = PrefixComparators.DoublePrefixComparator.computePrefix(Double.NegativeInfinity)
145149
val minValue = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MinValue)
146150
val maxValue = PrefixComparators.DoublePrefixComparator.computePrefix(Double.MaxValue)
147151
val zero = PrefixComparators.DoublePrefixComparator.computePrefix(0.0)
152+
val minusZero = PrefixComparators.DoublePrefixComparator.computePrefix(-0.0)
153+
154+
// null is greater than everything including NaN, when we need to treat it as the largest value.
155+
assert(PrefixComparators.DOUBLE.compare(largestNullPrefix, nan) === 1)
156+
// NaN is greater than the positive infinity.
148157
assert(PrefixComparators.DOUBLE.compare(nan, posInf) === 1)
149158
assert(PrefixComparators.DOUBLE.compare(posInf, maxValue) === 1)
150159
assert(PrefixComparators.DOUBLE.compare(maxValue, zero) === 1)
151160
assert(PrefixComparators.DOUBLE.compare(zero, minValue) === 1)
152161
assert(PrefixComparators.DOUBLE.compare(minValue, negInf) === 1)
153-
assert(PrefixComparators.DOUBLE.compare(negInf, nullValue) === 1)
162+
// null is smaller than everything including negative infinity, when we need to treat it as
163+
// the smallest value.
164+
assert(PrefixComparators.DOUBLE.compare(negInf, smallestNullPrefix) === 1)
165+
// 0.0 should be equal to -0.0.
166+
assert(PrefixComparators.DOUBLE.compare(zero, minusZero) === 0)
154167
}
155168
}

0 commit comments

Comments
 (0)