[SPARK-16802] [SQL] fix overflow in LongToUnsafeRowMap #14464
Conversation
| val idx = (key - minKey).toInt | ||
| if (idx >= 0 && key <= maxKey && array(idx) > 0) { | ||
| val idx = (key - minKey).toInt // could overflow | ||
| if (key >= minKey && key <= maxKey && array(idx) > 0) { |
There was a problem hiding this comment.
Yeah I see where this is going but I think this doesn't totally eliminate the problem. key - minKey could still overflow such that the int is positive and even >= minKey. It seems like we need to test the keys against each other as longs, and only then covert to an int to index into the array?
There was a problem hiding this comment.
I think having both key >= minKey and key <= maxKey could make sure that there is no overflow (because we already make sure that the range between minKey and maxKey is smaller than Int.MaxValue), then we can safely use (key - minKey).toInt
There was a problem hiding this comment.
Ah yeah OK this should be OK. I might suggest the following as a little simpler, but whatever:
if (key >= minKey && key <= maxKey) {
val value = array((key - minKey).toInt)
if (value > 0) {
return getRow(value, resultRow)
}
}
?
|
Test build #63133 has finished for PR 14464 at commit
|
|
LGTM |
|
Test build #63191 has finished for PR 14464 at commit
|
## What changes were proposed in this pull request? This patch fix the overflow in LongToUnsafeRowMap when the range of key is very wide (the key is much much smaller then minKey, for example, key is Long.MinValue, minKey is > 0). ## How was this patch tested? Added regression test (also for SPARK-16740) Author: Davies Liu <davies@databricks.com> Closes #14464 from davies/fix_overflow. (cherry picked from commit 9d4e621) Signed-off-by: Davies Liu <davies.liu@gmail.com>
What changes were proposed in this pull request?
This patch fix the overflow in LongToUnsafeRowMap when the range of key is very wide (the key is much much smaller then minKey, for example, key is Long.MinValue, minKey is > 0).
How was this patch tested?
Added regression test (also for SPARK-16740)