[SPARK-16922] [SPARK-17211] [SQL] make the address of values portable in LongToUnsafeRowMap #14927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @rxin @sitalkedia |
| @@ -448,7 +448,7 @@ private[execution] final class LongToUnsafeRowMap(val mm: TaskMemoryManager, cap | |||
| private def nextSlot(pos: Int): Int = (pos + 2) & mask | |||
|
|
|||
| private def getRow(address: Long, resultRow: UnsafeRow): UnsafeRow = { | |||
| val offset = address >>> SIZE_BITS | |||
| val offset = (address >>> SIZE_BITS) + Platform.LONG_ARRAY_OFFSET | |||
There was a problem hiding this comment.
This may sound a bit redundant. How about creating two methods:
private[this] def toOffset(address: Long): Long = (address >>> SIZE_BITS) + Platform.LONG_ARRAY_OFFSET
private[this] def toAdress(offset: Long): Long = (offset - Platform.LONG_ARRAY_OFFSET) << SIZE_BITSTo make it clearer what you are doing there. They should be inlined, so the performance overhead is minimal.
|
Test build #64804 has finished for PR 14927 at commit
|
|
Test build #64809 has finished for PR 14927 at commit
|
|
@davies are we making this assumption anywhere else in our unsafe code? |
|
@hvanhovell Has lgtm offline. Other contributors also confirm that this patch fix the bug, I'm going to merge this one into master and 2.0 branch. |
asfgit
pushed a commit
that referenced
this pull request
Sep 6, 2016
… in LongToUnsafeRowMap ## What changes were proposed in this pull request? In LongToUnsafeRowMap, we use offset of a value as pointer, stored in a array also in the page for chained values. The offset is not portable, because Platform.LONG_ARRAY_OFFSET will be different with different JVM Heap size, then the deserialized LongToUnsafeRowMap will be corrupt. This PR will change to use portable address (without Platform.LONG_ARRAY_OFFSET). ## How was this patch tested? Added a test case with random generated keys, to improve the coverage. But this test is not a regression test, that could require a Spark cluster that have at least 32G heap in driver or executor. Author: Davies Liu <[email protected]> Closes #14927 from davies/longmap. (cherry picked from commit f7e26d7) Signed-off-by: Davies Liu <[email protected]>
|
LGTM |
rishitesh
pushed a commit
to TIBCOSoftware/snappy-spark
that referenced
this pull request
Sep 26, 2016
… in LongToUnsafeRowMap ## What changes were proposed in this pull request? In LongToUnsafeRowMap, we use offset of a value as pointer, stored in a array also in the page for chained values. The offset is not portable, because Platform.LONG_ARRAY_OFFSET will be different with different JVM Heap size, then the deserialized LongToUnsafeRowMap will be corrupt. This PR will change to use portable address (without Platform.LONG_ARRAY_OFFSET). ## How was this patch tested? Added a test case with random generated keys, to improve the coverage. But this test is not a regression test, that could require a Spark cluster that have at least 32G heap in driver or executor. Author: Davies Liu <[email protected]> Closes apache#14927 from davies/longmap.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
In LongToUnsafeRowMap, we use offset of a value as pointer, stored in a array also in the page for chained values. The offset is not portable, because Platform.LONG_ARRAY_OFFSET will be different with different JVM Heap size, then the deserialized LongToUnsafeRowMap will be corrupt.
This PR will change to use portable address (without Platform.LONG_ARRAY_OFFSET).
How was this patch tested?
Added a test case with random generated keys, to improve the coverage. But this test is not a regression test, that could require a Spark cluster that have at least 32G heap in driver or executor.