[SPARK-26382][CORE] prefix comparator should handle -0.0 #23334

cloud-fan · 2018-12-17T08:47:17Z

What changes were proposed in this pull request?

This is kind of a followup of #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

cloud-fan · 2018-12-17T08:52:38Z

cc @dongjoon-hyun @viirya @kiszk @adoron

cloud-fan · 2018-12-17T08:53:53Z

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

    assert(PrefixComparators.DOUBLE.compare(prefix, doubleMaxPrefix) === 1)
  }

  test("double prefix comparator handles other special values properly") {
-    val nullValue = 0L
+    // See `SortPrefix.nulValue` for how we deal with nulls for float/double type


improve the test coverage a little bit.

viirya · 2018-12-17T09:24:13Z

By reading the PR description, sounds that this change is not for addressing an existing bug but just a safer guard, right?

SparkQA · 2018-12-17T13:11:24Z

Test build #100231 has finished for PR 23334 at commit 7329d4c.

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

SparkQA · 2018-12-17T13:20:13Z

Test build #100233 has finished for PR 23334 at commit 311917a.

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

cloud-fan · 2018-12-17T13:33:59Z

@viirya yes you are right

kiszk · 2018-12-17T17:32:26Z

LGTM pending jenkins
nit: For our future, it would be good to add a link to this PR from bullet 1 in the description of #23239

dongjoon-hyun · 2018-12-17T17:37:14Z

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

@@ -69,6 +69,8 @@ public static long computePrefix(byte[] bytes) {
     * 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;


Technically, value == -0.0 is true for both 0.0 and -0.0, but the current one is the simplest one for this normalization. We don't need to exclude 0.0 here. +1 for the logic.

dongjoon-hyun · 2018-12-17T17:42:49Z

Shall we use [CORE] instead of [SQL] since this touches only PrefixComparators in core?

dongjoon-hyun

+1, LGTM.

SparkQA · 2018-12-17T18:04:40Z

Test build #100243 has finished for PR 23334 at commit 8245bf4.

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

dongjoon-hyun · 2018-12-17T18:59:15Z

Although this is not a bug fix, can we have this in branch-2.4, @cloud-fan and @gatorsmile ? I'm +1 for backporting.

cloud-fan · 2018-12-18T03:13:38Z

@kiszk good idea, updated #23239

@dongjoon-hyun I'm fine with it. Feel free to merge :)

dongjoon-hyun · 2018-12-18T18:09:12Z

Thank you, @cloud-fan and @viirya , @HyukjinKwon , @kiszk .
Merged to master/branch-2.4 .

## What changes were proposed in this pull request? This is kind of a followup of #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 #23334 from cloud-fan/sort. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit befca98) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

## 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>

## 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> (cherry picked from commit befca98) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

cloud-fan changed the title ~~[SPARK-26382][] prefix sorter should handle -0.0~~ [SPARK-26382][SQL] prefix comparator should handle -0.0 Dec 17, 2018

cloud-fan commented Dec 17, 2018

View reviewed changes

prefix sorter should handle -0.0

311917a

cloud-fan force-pushed the sort branch from 7329d4c to 311917a Compare December 17, 2018 08:55

viirya approved these changes Dec 17, 2018

View reviewed changes

HyukjinKwon approved these changes Dec 17, 2018

View reviewed changes

fix typo

8245bf4

dongjoon-hyun reviewed Dec 17, 2018

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-26382][SQL] prefix comparator should handle -0.0~~ [SPARK-26382][CORE] prefix comparator should handle -0.0 Dec 17, 2018

dongjoon-hyun approved these changes Dec 17, 2018

View reviewed changes

cloud-fan mentioned this pull request Dec 18, 2018

[SPARK-26021][SQL][followup] only deal with NaN and -0.0 in UnsafeWriter #23239

Closed

asfgit closed this in befca98 Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-26382][CORE] prefix comparator should handle -0.0 #23334

[SPARK-26382][CORE] prefix comparator should handle -0.0 #23334

Uh oh!

cloud-fan commented Dec 17, 2018 •

edited

Loading

Uh oh!

cloud-fan commented Dec 17, 2018

Uh oh!

cloud-fan Dec 17, 2018

Uh oh!

viirya commented Dec 17, 2018

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

cloud-fan commented Dec 17, 2018

Uh oh!

kiszk commented Dec 17, 2018

Uh oh!

dongjoon-hyun Dec 17, 2018

Uh oh!

dongjoon-hyun commented Dec 17, 2018

Uh oh!

dongjoon-hyun left a comment

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

dongjoon-hyun commented Dec 17, 2018

Uh oh!

cloud-fan commented Dec 18, 2018

Uh oh!

dongjoon-hyun commented Dec 18, 2018

Uh oh!

Uh oh!

[SPARK-26382][CORE] prefix comparator should handle -0.0 #23334

[SPARK-26382][CORE] prefix comparator should handle -0.0 #23334

Uh oh!

Conversation

cloud-fan commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Dec 17, 2018

Uh oh!

cloud-fan Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

viirya commented Dec 17, 2018

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

cloud-fan commented Dec 17, 2018

Uh oh!

kiszk commented Dec 17, 2018

Uh oh!

dongjoon-hyun Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 17, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 17, 2018

Uh oh!

dongjoon-hyun commented Dec 17, 2018

Uh oh!

cloud-fan commented Dec 18, 2018

Uh oh!

dongjoon-hyun commented Dec 18, 2018

Uh oh!

Uh oh!

cloud-fan commented Dec 17, 2018 •

edited

Loading