[SPARK-3032][Shuffle] Fix key comparison integer overflow introduced sorting exception #2514

jerryshao · 2014-09-24T04:45:30Z

Previous key comparison in ExternalSorter will get wrong sorting result or exception when key comparison overflows, details can be seen in SPARK-3032. Here fix this and add a unit test to prove it.

srowen · 2014-09-24T04:47:14Z

core/src/main/scala/org/apache/spark/util/collection/ExternalSorter.scala

@@ -152,7 +152,7 @@ private[spark] class ExternalSorter[K, V, C](
    override def compare(a: K, b: K): Int = {
      val h1 = if (a == null) 0 else a.hashCode()
      val h2 = if (b == null) 0 else b.hashCode()
-      h1 - h2
+      Integer.compare(h1, h2)


This method does not exist in Java 6. There is an equivalent in Guava or you can just write the comparisons directly.

OK, thanks Sean.

SparkQA · 2014-09-24T04:49:17Z

QA tests have started for PR 2514 at commit fa2a08f.

This patch merges cleanly.

SparkQA · 2014-09-24T05:19:18Z

QA tests have started for PR 2514 at commit 83acb38.

This patch merges cleanly.

mateiz · 2014-09-24T05:27:57Z

core/src/main/scala/org/apache/spark/util/collection/ExternalSorter.scala

@@ -152,7 +152,7 @@ private[spark] class ExternalSorter[K, V, C](
    override def compare(a: K, b: K): Int = {
      val h1 = if (a == null) 0 else a.hashCode()
      val h2 = if (b == null) 0 else b.hashCode()
-      h1 - h2
+      if (h1 < h2) -1 else if (h1 == h2) 0 else 1


Don't use this, use Integer.compare instead. Reason is Java will likely optimize the latter one. You can fix it in ExternalAppendOnlyMap too.

So what about Java 6 compatibility?

@mateiz per my comment, that would no longer run in Java 6 as Integer.compare doesn't exist before Java 7.

Alright, we can keep it as is then.

SparkQA · 2014-09-24T05:57:00Z

QA tests have finished for PR 2514 at commit fa2a08f.

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

SparkQA · 2014-09-24T05:57:03Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20741/

SparkQA · 2014-09-24T06:27:48Z

QA tests have finished for PR 2514 at commit 83acb38.

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

SparkQA · 2014-09-24T06:27:52Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20742/

SparkQA · 2014-09-25T08:14:26Z

QA tests have started for PR 2514 at commit 01911e6.

This patch merges cleanly.

jerryshao · 2014-09-25T08:33:10Z

Hi Matei, I modify the unit test to reproduce the exception. Seems it is difficult to reproduce this exception with small dataset manually, as this exception is unreliable.

Here is the reason searched from stack overflow

the exception behavior is unreliable: As long as you have small data sets (so small that a generated run may never gallop, as MIN_GALLOP is 7) or the generated runs always coincidentally generate a merge that never gallops, you will never receive the exception. Thus, without further reviewing the gallopRight method, we can come to the conclusion that you cannot rely on the exception: It may never be thrown no matter how wrong your comparator is.

So here I generate 1m random integer values to reproduce the exception. Seems in my local test with above 1000 rounds of test, this exception can always be produced. But it cannot be logically proved and still have chance to not throw exception.

Also I tested with 1k random integer plus some large data like Integer.MaxValue and Integer.MinValue, hard to reproduce this exception. And with 10k dataset, 1/3 of chance to get the exception.

I think unless someone familiar with TimSort can manually create effectively small dataset, potentially this unit test may fail.

So would you give me some suggestions? Thanks a lot.

SparkQA · 2014-09-25T09:08:48Z

QA tests have finished for PR 2514 at commit 01911e6.

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

AmplabJenkins · 2014-09-25T09:08:51Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20788/

mateiz · 2014-09-25T20:45:57Z

If you have a test that reproduces it in 1000 runs, it's fine. I'd improve it by seeding a random generator with a fixed seed that you saw produce the problem. TimSort is deterministic, so it will always throw it in that case (though this may break if we change the implementation).

mateiz · 2014-09-25T20:46:47Z

core/src/test/scala/org/apache/spark/util/collection/ExternalSorterSuite.scala

+    sc = new SparkContext("local-cluster[1,1,512]", "test", conf)
+
+    // Using wrongHashOrdering to show integer overflow introduced exception.
+    val rand = new Random


Just seed this at a given value that you see causes the exception to occur.

SparkQA · 2014-09-28T05:50:29Z

QA tests have started for PR 2514 at commit 6f3c302.

This patch merges cleanly.

jerryshao · 2014-09-28T05:51:13Z

Hi Matei, thanks a lot for your suggestions. I've updated the code with fixed seed. Would you mind taking a look at this? Thanks a lot.

SparkQA · 2014-09-28T06:45:39Z

QA tests have finished for PR 2514 at commit 6f3c302.

This patch fails unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class IndexedRecordToJavaConverter extends Converter[IndexedRecord, JMap[String, Any]]
- class AvroWrapperToJavaConverter extends Converter[Any, Any]

AmplabJenkins · 2014-09-28T06:45:42Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20928/

jerryshao · 2014-09-28T07:08:16Z

Jenkins, retest this please.

SparkQA · 2014-09-28T07:14:26Z

QA tests have started for PR 2514 at commit 6f3c302.

This patch merges cleanly.

SparkQA · 2014-09-28T08:22:42Z

QA tests have finished for PR 2514 at commit 6f3c302.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class IndexedRecordToJavaConverter extends Converter[IndexedRecord, JMap[String, Any]]
- class AvroWrapperToJavaConverter extends Converter[Any, Any]

AmplabJenkins · 2014-09-28T08:22:46Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/20931/

…sorting exception Previous key comparison in `ExternalSorter` will get wrong sorting result or exception when key comparison overflows, details can be seen in [SPARK-3032](https://issues.apache.org/jira/browse/SPARK-3032). Here fix this and add a unit test to prove it. Author: jerryshao <saisai.shao@intel.com> Closes #2514 from jerryshao/SPARK-3032 and squashes the following commits: 6f3c302 [jerryshao] Improve the unit test according to comments 01911e6 [jerryshao] Change the test to show the contract violate exception 83acb38 [jerryshao] Minor changes according to comments fa2a08f [jerryshao] Fix key comparison integer overflow introduced sorting exception (cherry picked from commit dab1b0a) Signed-off-by: Matei Zaharia <matei@databricks.com>

mateiz · 2014-09-29T18:26:09Z

Thanks Saisai, this looks good. I've merged it.

Fix key comparison integer overflow introduced sorting exception

fa2a08f

srowen reviewed Sep 24, 2014
View reviewed changes

Minor changes according to comments

83acb38

mateiz reviewed Sep 24, 2014
View reviewed changes

Change the test to show the contract violate exception

01911e6

mateiz reviewed Sep 25, 2014
View reviewed changes

Improve the unit test according to comments

6f3c302

asfgit closed this in dab1b0a Sep 29, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-3032][Shuffle] Fix key comparison integer overflow introduced sorting exception #2514

[SPARK-3032][Shuffle] Fix key comparison integer overflow introduced sorting exception #2514

jerryshao commented Sep 24, 2014

srowen Sep 24, 2014

jerryshao Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

mateiz Sep 24, 2014

jerryshao Sep 24, 2014

srowen Sep 24, 2014

mateiz Sep 25, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 25, 2014

jerryshao commented Sep 25, 2014

SparkQA commented Sep 25, 2014

AmplabJenkins commented Sep 25, 2014

mateiz commented Sep 25, 2014

mateiz Sep 25, 2014

SparkQA commented Sep 28, 2014

jerryshao commented Sep 28, 2014

SparkQA commented Sep 28, 2014

AmplabJenkins commented Sep 28, 2014

jerryshao commented Sep 28, 2014

SparkQA commented Sep 28, 2014

SparkQA commented Sep 28, 2014

AmplabJenkins commented Sep 28, 2014

mateiz commented Sep 29, 2014

[SPARK-3032][Shuffle] Fix key comparison integer overflow introduced sorting exception #2514

[SPARK-3032][Shuffle] Fix key comparison integer overflow introduced sorting exception #2514

Conversation

jerryshao commented Sep 24, 2014

srowen Sep 24, 2014

Choose a reason for hiding this comment

jerryshao Sep 24, 2014

Choose a reason for hiding this comment

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

mateiz Sep 24, 2014

Choose a reason for hiding this comment

jerryshao Sep 24, 2014

Choose a reason for hiding this comment

srowen Sep 24, 2014

Choose a reason for hiding this comment

mateiz Sep 25, 2014

Choose a reason for hiding this comment

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 24, 2014

SparkQA commented Sep 25, 2014

jerryshao commented Sep 25, 2014

SparkQA commented Sep 25, 2014

AmplabJenkins commented Sep 25, 2014

mateiz commented Sep 25, 2014

mateiz Sep 25, 2014

Choose a reason for hiding this comment

SparkQA commented Sep 28, 2014

jerryshao commented Sep 28, 2014

SparkQA commented Sep 28, 2014

AmplabJenkins commented Sep 28, 2014

jerryshao commented Sep 28, 2014

SparkQA commented Sep 28, 2014

SparkQA commented Sep 28, 2014

AmplabJenkins commented Sep 28, 2014

mateiz commented Sep 29, 2014