[SPARK-5201][CORE] deal with int overflow in the ParallelCollectionRDD.slice method #4002

advancedxy · 2015-01-12T07:49:34Z

There is an int overflow in the ParallelCollectionRDD.slice method. That's originally reported by SaintBacchus.

sc.makeRDD(1 to (Int.MaxValue)).count       // result = 0
sc.makeRDD(1 to (Int.MaxValue - 1)).count   // result = 2147483646 = Int.MaxValue - 1
sc.makeRDD(1 until (Int.MaxValue)).count    // result = 2147483646 = Int.MaxValue - 1

see #2874 for more details.
This pr try to fix the overflow. However, There's another issue I don't address.

val largeRange = Int.MinValue to Int.MaxValue
largeRange.length // throws java.lang.IllegalArgumentException: -2147483648 to 2147483647 by 1: seqs cannot contain more than Int.MaxValue elements.

So, the range we feed to sc.makeRDD cannot contain more than Int.MaxValue elements. This is the limitation of Scala. However I think we may want to support that kind of range. But the fix is beyond this pr.

@srowen @andrewor14 would you mind take a look at this pr?

… inclusive and the end of the range is (Int.MaxValue or Int.MinValue), we should use inclusive range instead of exclusive

AmplabJenkins · 2015-01-12T07:52:10Z

Can one of the admins verify this patch?

srowen · 2015-01-12T12:15:24Z

core/src/main/scala/org/apache/spark/rdd/ParallelCollectionRDD.scala

Maybe I'm missing it but why is IntMinValue a special case here? Also the ({ on the next line is redundant. Just one is needed. sign isn't terribly descriptive here either.

Try to convert this inclusive range

-2 to Int.MinValue by -1

to exclusive range will be

-2L until -1L + Int.MinValue by -1

-1 + Int.MinValue will overflow.

As for sign, which name would you recommend? How about inclusiveRangeWithIntBoundary?

Ah right, the range can go backwards. Yeah, something like needsInclusiveRange or exceptionalBoundary or something.

ok. Will change the name.
As for redundant ({, there is a infix operator toSeq, so I prefer the redundant one. And the previous code used ({

andrewor14 · 2015-01-12T15:53:32Z

ok to test

SparkQA · 2015-01-12T15:57:36Z

Test build #25419 has started for PR 4002 at commit 651c959.

This patch merges cleanly.

SparkQA · 2015-01-12T17:05:41Z

Test build #25419 has finished for PR 4002 at commit 651c959.

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

AmplabJenkins · 2015-01-12T17:05:44Z

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

andrewor14 · 2015-01-12T19:35:54Z

core/src/main/scala/org/apache/spark/rdd/ParallelCollectionRDD.scala

this need not be specific to Int.MaxValue and Int.MinValue. We can just do a special case for inclusive range in general and I think the code will be significantly more straightforward to read. I'll explain below.

…usive range, hence eliminate the need to check Int.MaxValue or Int.MinValue.

SparkQA · 2015-01-14T02:27:31Z

Test build #25499 has started for PR 4002 at commit 96265a1.

This patch merges cleanly.

advancedxy · 2015-01-14T02:34:59Z

@andrewor14 would you mind to take a look again?

 positions(r.length, numSlices).zipWithIndex.map({ case ((start, end), index) => // the redundant "({" is necessary, as there is an infix operator
          // If the range is inclusive, use inclusive range for the last slice
          if (r.isInclusive && index == numSlices - 1) {
            new Range.Inclusive(r.start + start * r.step, r.end, r.step)
          }
          else {
            new Range(r.start + start * r.step, r.start + end * r.step, r.step)
          }
        }).toSeq.asInstanceOf[Seq[Seq[T]]]  // the toSeq and casting is necessary, otherwise it won't compile due to type mismatch

I update some docs. There may be some wrongly chosen words as I am not a native speaker. Please correct me if you see any.

SparkQA · 2015-01-14T03:38:17Z

Test build #25499 has finished for PR 4002 at commit 96265a1.

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

AmplabJenkins · 2015-01-14T03:38:21Z

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

srowen · 2015-01-14T11:44:02Z

core/src/main/scala/org/apache/spark/SparkContext.scala

Is it necessary for callers to know this? It seems like purely an implementation detail that could be documented inside the function later. Same for the scaladoc later.
I would say "the sub Ranges in each slice are exclusive"
Otherwise at a glance this looks good.

Thanks for your comment. I will update the docs if we decide to add them.

I agree. I would rather leave this out. I'll delete this when I merge.

advancedxy · 2015-01-15T13:49:16Z

ping @andrewor14

andrewor14 · 2015-01-16T17:20:13Z

Thanks @advancedxy I'm merging this into master and 1.2

…D.slice method There is an int overflow in the ParallelCollectionRDD.slice method. That's originally reported by SaintBacchus. ``` sc.makeRDD(1 to (Int.MaxValue)).count // result = 0 sc.makeRDD(1 to (Int.MaxValue - 1)).count // result = 2147483646 = Int.MaxValue - 1 sc.makeRDD(1 until (Int.MaxValue)).count // result = 2147483646 = Int.MaxValue - 1 ``` see #2874 for more details. This pr try to fix the overflow. However, There's another issue I don't address. ``` val largeRange = Int.MinValue to Int.MaxValue largeRange.length // throws java.lang.IllegalArgumentException: -2147483648 to 2147483647 by 1: seqs cannot contain more than Int.MaxValue elements. ``` So, the range we feed to sc.makeRDD cannot contain more than Int.MaxValue elements. This is the limitation of Scala. However I think we may want to support that kind of range. But the fix is beyond this pr. srowen andrewor14 would you mind take a look at this pr? Author: Ye Xianjin <[email protected]> Closes #4002 from advancedxy/SPARk-5201 and squashes the following commits: 96265a1 [Ye Xianjin] Update slice method comment and some responding docs. e143d7a [Ye Xianjin] Update inclusive range check for splitting inclusive range. b3f5577 [Ye Xianjin] We can include the last element in the last slice in general for inclusive range, hence eliminate the need to check Int.MaxValue or Int.MinValue. 7d39b9e [Ye Xianjin] Convert the two cases pattern matching to one case. 651c959 [Ye Xianjin] rename sign to needsInclusiveRange. add some comments 196f8a8 [Ye Xianjin] Add test cases for ranges end with Int.MaxValue or Int.MinValue e66e60a [Ye Xianjin] Deal with inclusive and exclusive ranges in one case. If the range is inclusive and the end of the range is (Int.MaxValue or Int.MinValue), we should use inclusive range instead of exclusive

advancedxy added 2 commits January 12, 2015 15:28

Deal with inclusive and exclusive ranges in one case. If the range is…

e66e60a

… inclusive and the end of the range is (Int.MaxValue or Int.MinValue), we should use inclusive range instead of exclusive

Add test cases for ranges end with Int.MaxValue or Int.MinValue

196f8a8

advancedxy changed the title ~~[Spark-5201] deal with int overflow in the ParallelCollectionRDD.slice method~~ [SPARK-5201][CORE] deal with int overflow in the ParallelCollectionRDD.slice method Jan 12, 2015

srowen reviewed Jan 12, 2015
View reviewed changes

rename sign to needsInclusiveRange. add some comments

651c959

andrewor14 reviewed Jan 12, 2015
View reviewed changes

advancedxy added 4 commits January 13, 2015 18:28

Convert the two cases pattern matching to one case.

7d39b9e

We can include the last element in the last slice in general for incl…

b3f5577

…usive range, hence eliminate the need to check Int.MaxValue or Int.MinValue.

Update inclusive range check for splitting inclusive range.

e143d7a

Update slice method comment and some responding docs.

96265a1

srowen reviewed Jan 14, 2015
View reviewed changes

advancedxy closed this Jan 15, 2015

advancedxy reopened this Jan 15, 2015

asfgit closed this in e200ac8 Jan 16, 2015

[SPARK-5201][CORE] deal with int overflow in the ParallelCollectionRDD.slice method #4002

[SPARK-5201][CORE] deal with int overflow in the ParallelCollectionRDD.slice method #4002

Uh oh!

Conversation

advancedxy commented Jan 12, 2015

Uh oh!

AmplabJenkins commented Jan 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Jan 12, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!

AmplabJenkins commented Jan 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 14, 2015

Uh oh!

advancedxy commented Jan 14, 2015

Uh oh!

SparkQA commented Jan 14, 2015

Uh oh!

AmplabJenkins commented Jan 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advancedxy commented Jan 15, 2015

Uh oh!

andrewor14 commented Jan 16, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants