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
Next Next commit
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
  • Loading branch information
@advancedxy
advancedxy committed Jan 12, 2015
commit e66e60a9f2f37a7845ab09a165ee960f830d7b4e
16 changes: 5 additions & 11 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 @@ -127,18 +127,12 @@ private object ParallelCollectionRDD {
})
}
seq match {
case r: Range.Inclusive => {
val sign = if (r.step < 0) {
-1
} else {
1
}
slice(new Range(
r.start, r.end + sign, r.step).asInstanceOf[Seq[T]], numSlices)
}
case r: Range => {
positions(r.length, numSlices).map({
case (start, end) =>
val sign = r.isInclusive && (r.end == Int.MaxValue || r.end == Int.MinValue)
Copy link
Member

@srowen srowen Jan 12, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@advancedxy advancedxy Jan 12, 2015

Choose a reason for hiding this comment

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

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?

Copy link
Member

@srowen srowen Jan 12, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@advancedxy advancedxy Jan 12, 2015

Choose a reason for hiding this comment

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

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 ({

positions(r.length, numSlices).zipWithIndex.map({
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.

not your code, but style should be .map { ... }

case ((start, end), index) if sign && index == numSlices - 1 =>
new Range.Inclusive(r.start + start * r.step, r.end, r.step)
case ((start, end), _) =>
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]]]
}
Expand Down