-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-18458][CORE] Fix signed integer overflow problem at an expression in RadixSort.java #15907
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.
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
Changes from 4 commits
3821a52
472c0c3
ae6820a
ef54e8a
022e5b3
3f9efdb
f2e2079
591626e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ import org.apache.spark.util.collection.Sorter | |
| import org.apache.spark.util.random.XORShiftRandom | ||
|
|
||
| class RadixSortSuite extends SparkFunSuite with Logging { | ||
| private val N = 10000 // scale this down for more readable results | ||
| private val N = 10000.toLong // scale this down for more readable results | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Write
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I will do that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
|
||
| /** | ||
| * Describes a type of sort to test, e.g. two's complement descending. Each sort type has | ||
|
|
@@ -73,22 +73,22 @@ class RadixSortSuite extends SparkFunSuite with Logging { | |
| }, | ||
| 2, 4, false, false, true)) | ||
|
|
||
| private def generateTestData(size: Int, rand: => Long): (Array[JLong], LongArray) = { | ||
| val ref = Array.tabulate[Long](size) { i => rand } | ||
| val extended = ref ++ Array.fill[Long](size)(0) | ||
| private def generateTestData(size: Long, rand: => Long): (Array[JLong], LongArray) = { | ||
| val ref = Array.tabulate[Long](size.toInt) { i => rand } | ||
| val extended = ref ++ Array.fill[Long](size.toInt)(0) | ||
| (ref.map(i => new JLong(i)), new LongArray(MemoryBlock.fromLongArray(extended))) | ||
| } | ||
|
|
||
| private def generateKeyPrefixTestData(size: Int, rand: => Long): (LongArray, LongArray) = { | ||
| val ref = Array.tabulate[Long](size * 2) { i => rand } | ||
| val extended = ref ++ Array.fill[Long](size * 2)(0) | ||
| private def generateKeyPrefixTestData(size: Long, rand: => Long): (LongArray, LongArray) = { | ||
| val ref = Array.tabulate[Long]((size * 2).toInt) { i => rand } | ||
| val extended = ref ++ Array.fill[Long]((size * 2).toInt)(0) | ||
| (new LongArray(MemoryBlock.fromLongArray(ref)), | ||
| new LongArray(MemoryBlock.fromLongArray(extended))) | ||
| } | ||
|
|
||
| private def collectToArray(array: LongArray, offset: Int, length: Int): Array[Long] = { | ||
| private def collectToArray(array: LongArray, offset: Int, length: Long): Array[Long] = { | ||
| var i = 0 | ||
| val out = new Array[Long](length) | ||
| val out = new Array[Long](length.toInt) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add a checkedCast function similar to Guava's: https://google.github.io/guava/releases/19.0/api/docs/com/google/common/primitives/Ints.html#checkedCast(long) and use those for all the toInt downcasts. Otherwise it'd be annoying to get mysterious negative integer exceptions.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! done in these files.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you forget to push?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I put this comment before pushed. Now, you can see it |
||
| while (i < length) { | ||
| out(i) = array.get(offset + i) | ||
| i += 1 | ||
|
|
@@ -107,10 +107,10 @@ class RadixSortSuite extends SparkFunSuite with Logging { | |
| } | ||
| } | ||
|
|
||
| private def referenceKeyPrefixSort(buf: LongArray, lo: Int, hi: Int, refCmp: PrefixComparator) { | ||
| private def referenceKeyPrefixSort(buf: LongArray, lo: Long, hi: Long, refCmp: PrefixComparator) { | ||
| val sortBuffer = new LongArray(MemoryBlock.fromLongArray(new Array[Long](buf.size().toInt))) | ||
| new Sorter(new UnsafeSortDataFormat(sortBuffer)).sort( | ||
| buf, lo, hi, new Comparator[RecordPointerAndKeyPrefix] { | ||
| buf, lo.toInt, hi.toInt, new Comparator[RecordPointerAndKeyPrefix] { | ||
| override def compare( | ||
| r1: RecordPointerAndKeyPrefix, | ||
| r2: RecordPointerAndKeyPrefix): Int = { | ||
|
|
@@ -156,7 +156,7 @@ class RadixSortSuite extends SparkFunSuite with Logging { | |
| val (ref, buffer) = generateTestData(N, rand.nextLong) | ||
| Arrays.sort(ref, toJavaComparator(sortType.referenceComparator)) | ||
| val outOffset = RadixSort.sort( | ||
| buffer, N, sortType.startByteIdx, sortType.endByteIdx, | ||
| buffer, N.toLong, sortType.startByteIdx, sortType.endByteIdx, | ||
| sortType.descending, sortType.signed) | ||
| val result = collectToArray(buffer, outOffset, N) | ||
| assert(ref.view == result.view) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,7 +80,7 @@ class SortBenchmark extends BenchmarkBase { | |
| } | ||
| val buf = new LongArray(MemoryBlock.fromLongArray(array)) | ||
| timer.startTiming() | ||
| RadixSort.sort(buf, size, 0, 7, false, false) | ||
| RadixSort.sort(buf, size.toLong, 0, 7, false, false) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need these toLongs? Doesn't scala do the cast implicitly?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I wrote before, I wanted to emphasis the difference of types between caller and callee. |
||
| timer.stopTiming() | ||
| } | ||
| benchmark.addTimerCase("radix sort two bytes") { timer => | ||
|
|
@@ -92,7 +92,7 @@ class SortBenchmark extends BenchmarkBase { | |
| } | ||
| val buf = new LongArray(MemoryBlock.fromLongArray(array)) | ||
| timer.startTiming() | ||
| RadixSort.sort(buf, size, 0, 7, false, false) | ||
| RadixSort.sort(buf, size.toLong, 0, 7, false, false) | ||
| timer.stopTiming() | ||
| } | ||
| benchmark.addTimerCase("radix sort eight bytes") { timer => | ||
|
|
@@ -104,13 +104,13 @@ class SortBenchmark extends BenchmarkBase { | |
| } | ||
| val buf = new LongArray(MemoryBlock.fromLongArray(array)) | ||
| timer.startTiming() | ||
| RadixSort.sort(buf, size, 0, 7, false, false) | ||
| RadixSort.sort(buf, size.toLong, 0, 7, false, false) | ||
| timer.stopTiming() | ||
| } | ||
| benchmark.addTimerCase("radix sort key prefix array") { timer => | ||
| val (_, buf2) = generateKeyPrefixTestData(size, rand.nextLong) | ||
| timer.startTiming() | ||
| RadixSort.sortKeyPrefixArray(buf2, 0, size, 0, 7, false, false) | ||
| RadixSort.sortKeyPrefixArray(buf2, 0.toLong, size.toLong, 0, 7, false, false) | ||
| timer.stopTiming() | ||
| } | ||
| benchmark.run() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again doesn't java do the upcast implicitly?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is OK to rely on implicit cast, done.