Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
advancedxy wants to merge 7 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e66e60a
Deal with inclusive and exclusive ranges in one case. If the range is…
advancedxy Jan 12, 2015
196f8a8
Add test cases for ranges end with Int.MaxValue or Int.MinValue
advancedxy Jan 12, 2015
651c959
rename sign to needsInclusiveRange. add some comments
advancedxy Jan 12, 2015
7d39b9e
Convert the two cases pattern matching to one case.
advancedxy Jan 13, 2015
b3f5577
We can include the last element in the last slice in general for incl…
advancedxy Jan 14, 2015
e143d7a
Update inclusive range check for splitting inclusive range.
advancedxy Jan 14, 2015
96265a1
Update slice method comment and some responding docs.
advancedxy Jan 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Convert the two cases pattern matching to one case.
  • Loading branch information
@advancedxy
advancedxy committed Jan 13, 2015
commit 7d39b9e9b9c0582993eaf585899ec972a1bb9a17
11 changes: 6 additions & 5 deletions core/src/main/scala/org/apache/spark/rdd/ParallelCollectionRDD.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,14 @@ private object ParallelCollectionRDD {
case r: Range => {
// 1 to Int.MaxValue and (-2 to Int.MinValue by -1) can trigger exclusive range int overflow
val needsInclusiveRange = r.isInclusive && (r.end == Int.MaxValue || r.end == Int.MinValue)
Copy link
Contributor

@andrewor14 andrewor14 Jan 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

positions(r.length, numSlices).zipWithIndex.map({
// we need an inclusive range to avoid int overflow and setting the last range to be
// inclusive is suffice
case ((start, end), index) if needsInclusiveRange && index == numSlices - 1 =>
positions(r.length, numSlices).zipWithIndex.map({ case ((start, end), index) =>
// If the range needs to be inclusive, include the last element in the last slice
if (needsInclusiveRange && index == numSlices - 1) {
new Range.Inclusive(r.start + start * r.step, r.end, r.step)
case ((start, end), _) =>
}
else {
new Range(r.start + start * r.step, r.start + end * r.step, r.step)
Copy link
Contributor

@andrewor14 andrewor14 Jan 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to match multiple cases here if we just ignore the index for the non-inclusive case. I think it's sufficient to do

positions(...).zipWithIndex.map { case ((start, end), index) =>
  // If the range is inclusive, include the last element in the last slice
  if (r.isInclusive && index == numSlices - 1) {
    new Range.Inclusive(r.start + start * r.step, r.end, r.step)
  } else {
    new Range(... as before ...)
  }
}

Copy link
Contributor Author

@advancedxy advancedxy Jan 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No doubt that your version is more straightforward than mine. When I wrote my code, I didn't consider splitting normal inclusive range using inclusive range. However the benefit of my implementation is that the splitting result will be same as in the master for normal inclusive ranges. I wonder there may be some spark code rely on the exclusive range output. And of course, I think we should update the corresponding document for this kind of change.

I will covert the pattern matching to one case and update the implementation when we decided which one fits better.

Copy link
Contributor

@andrewor14 andrewor14 Jan 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as long as we don't change the behavior it's preferrable to rewrite it in a readable manner. Here it's pretty clear to me that if the range is inclusive we should include the last element in the last slice, regardless of whether the range ends in a special value like Int.MaxValue.

}
}).toSeq.asInstanceOf[Seq[Seq[T]]]
}
case nr: NumericRange[_] => {
Expand Down