SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException #4804

hotou · 2015-02-27T04:32:41Z

Fix TimSort bug which causes a ArrayOutOfBoundsException.

Using the proposed fix here
http://envisage-project.eu/proving-android-java-and-python-sorting-algorithm-is-broken-and-how-to-fix-it/

AmplabJenkins · 2015-02-27T04:37:09Z

Can one of the admins verify this patch?

rxin · 2015-02-27T04:39:15Z

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

@@ -65,6 +65,12 @@ class SorterSuite extends FunSuite {
    }
  }

+  test("bug of TimSort") {


would be good to provide a link to the original blog post

and would be better to include the JIRA ticket number, i.e.

test("SPARK-5984 TimSort bug") { ... }

rxin · 2015-02-27T04:44:09Z

Thanks for submitting the patch. I left some minor comments.

rxin · 2015-02-27T04:44:47Z

core/src/test/java/org/apache/spark/util/collection/TestTimSort.java

+ * This codes generates a int array which fails the standard TimSort, Borrowed from
+ * the reporter of this bug.
+ *
+ * http://www.envisage-project.eu/timsort-specification-and-verification/


we should also link to the github repository with an explicit license https://github.com/abstools/java-timsort-bug

Yes that's good practice. For completeness, because this AL2 and because there is no additional NOTICE file, we don't need to do anything else to our LICENSE or NOTICE file.

hotou · 2015-02-28T01:56:46Z

@rxin @srowen Thanks for the review, I updated the comments and licensed info etc.

rxin · 2015-02-28T02:10:02Z

Jenkins, test this please.

SparkQA · 2015-02-28T02:13:05Z

Test build #28105 has started for PR 4804 at commit 4d95f75.

This patch merges cleanly.

SparkQA · 2015-02-28T02:13:10Z

Test build #28105 has finished for PR 4804 at commit 4d95f75.

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

AmplabJenkins · 2015-02-28T02:13:11Z

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

hotou · 2015-02-28T02:16:04Z

Ah. I guess I have to have license file header in .java, not just linked to it

rxin · 2015-02-28T02:19:21Z

core/src/test/java/org/apache/spark/util/collection/TestTimSort.java

+
+import java.util.*;
+
+/*


you need to put this in the beginning of the file, i.e. before the package definition

hotou · 2015-02-28T02:20:22Z

ok

srowen · 2015-02-28T02:24:54Z

core/src/test/java/org/apache/spark/util/collection/TestTimSort.java

+ * https://github.com/abstools/java-timsort-bug
+ *
+ * Licensed under Apache License 2.0
+ * https://github.com/abstools/java-timsort-bug/blob/master/LICENSE


If this test code is your own work, then this statement is redundant with the license header, so would be removed.

But it's copied from the project above right? then you can't write a license header here that says it was licensed to the ASF. If anything we would reproduce the plain vanilla AL2 stanza from the plain AL2 license text up above in the file's license header.

That or not copy this test code. This Java code needs a bit more style work to match coding practices here.

Well, it's not a exact copy, I made changes to the original codes.

Do you guys have a IntelliJ style that I can import ?

It looks like it's almost entirely the code from the third party site. The right-est thing to do is actually begin this file with the standard AL2 stanza:

Copyright 2015 [the author's name] Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.

... since that is substantially the license of the work in the file. I believe the build's check for this stuff will accept this, or should. It can be followed with a comment that the work has been modified from its original form.

I don't think it's crazy to omit this either, though always nice to have tests.

THere's no standard IJ config but I'll point out some things that could be touched up.

hotou · 2015-02-28T03:48:34Z

@srowen I did what you recommended here. This passed the rat test on my machine at least

srowen · 2015-02-28T03:55:15Z

OK, I apologize for belaboring this and hassle with the incorrect suggestion earlier. But I think we may have to do one more thing for the licensing to get it right, and we should. I believe we need an entry in our own LICENSE file after all, given the situation. You can see one for the copied TimSort. I'd just add it below that. Then it really does look good from a license perspective, AFAIK.

hotou · 2015-02-28T04:07:37Z

@srowen Sounds good, done.

srowen · 2015-02-28T15:36:31Z

Thanks @hotou this really looks buttoned up now. Go for it @rxin if it looks good to you or else I will merge tomorrow.

rxin · 2015-02-28T18:55:00Z

Let's give @aarondav a chance to look at this also since he's the guy that introduced the timsort.

aarondav · 2015-02-28T19:26:40Z

LGTM as well.

rxin · 2015-03-01T02:56:49Z

Thanks. I've merged this.

Fix TimSort bug which causes a ArrayOutOfBoundsException. Using the proposed fix here http://envisage-project.eu/proving-android-java-and-python-sorting-algorithm-is-broken-and-how-to-fix-it/ Author: Evan Yu <ehotou@gmail.com> Closes #4804 from hotou/SPARK-5984 and squashes the following commits: 3421b6c [Evan Yu] SPARK-5984: Add info to LICENSE e61c6b8 [Evan Yu] SPARK-5984: Fix license and document 6ccc280 [Evan Yu] SPARK-5984: Add License header to file e06c0d2 [Evan Yu] SPARK-5984: Add License header to file 4d95f75 [Evan Yu] SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException 479a106 [Evan Yu] SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException (cherry picked from commit 643300a) Signed-off-by: Reynold Xin <rxin@databricks.com>

hotou · 2015-03-01T03:21:36Z

Thanks for the review guys

SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException

479a106

rxin reviewed Feb 27, 2015
View reviewed changes

SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException

4d95f75

SPARK-5984: Add License header to file

e06c0d2

rxin reviewed Feb 28, 2015
View reviewed changes

SPARK-5984: Add License header to file

6ccc280

srowen reviewed Feb 28, 2015
View reviewed changes

SPARK-5984: Fix license and document

e61c6b8

SPARK-5984: Add info to LICENSE

3421b6c

asfgit closed this in 643300a Mar 1, 2015

dongjoon-hyun mentioned this pull request Jan 2, 2019

[SPARK-26306][TEST][BUILD] More memory to de-flake SorterSuite #23425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException #4804

SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException #4804

hotou commented Feb 27, 2015

AmplabJenkins commented Feb 27, 2015

rxin Feb 27, 2015

rxin Feb 27, 2015

rxin commented Feb 27, 2015

rxin Feb 27, 2015

srowen Feb 27, 2015

hotou commented Feb 28, 2015

rxin commented Feb 28, 2015

SparkQA commented Feb 28, 2015

SparkQA commented Feb 28, 2015

AmplabJenkins commented Feb 28, 2015

hotou commented Feb 28, 2015

rxin Feb 28, 2015

hotou commented Feb 28, 2015

srowen Feb 28, 2015

hotou Feb 28, 2015

srowen Feb 28, 2015

hotou commented Feb 28, 2015

srowen commented Feb 28, 2015

hotou commented Feb 28, 2015

srowen commented Feb 28, 2015

rxin commented Feb 28, 2015

aarondav commented Feb 28, 2015

rxin commented Mar 1, 2015

hotou commented Mar 1, 2015

SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException #4804

SPARK-5984: Fix TimSort bug causes ArrayOutOfBoundsException #4804

Conversation

hotou commented Feb 27, 2015

AmplabJenkins commented Feb 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxin commented Feb 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotou commented Feb 28, 2015

rxin commented Feb 28, 2015

SparkQA commented Feb 28, 2015

SparkQA commented Feb 28, 2015

AmplabJenkins commented Feb 28, 2015

hotou commented Feb 28, 2015

Choose a reason for hiding this comment

hotou commented Feb 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotou commented Feb 28, 2015

srowen commented Feb 28, 2015

hotou commented Feb 28, 2015

srowen commented Feb 28, 2015

rxin commented Feb 28, 2015

aarondav commented Feb 28, 2015

rxin commented Mar 1, 2015

hotou commented Mar 1, 2015