-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-9483] Fix UTF8String.getPrefix for big-endian. #7902
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
Conversation
Previous code assumed little-endian.
|
/cc @davies |
|
Jenkins, this is ok to test. |
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.
can we move the result of ByteOrder.nativeOrder into a static variable?
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.
Will do
|
@mtbrandy not 100% sure, but it might be more clear if you just have a top level branch based on endianness. |
|
@rxin, regarding top-level branch -- I can do that, but was just trying to minimize duplicate code. |
|
+1 for moving the endianness check to top level, otherwise LGTM. |
|
ok, I'll modify to move the endianness check up. |
- Introduce static byteOrder variable - Branch on byteOrder at top-level for readability.
|
Test build #39580 has finished for PR 7902 at commit
|
|
@davies I will let you merge this. |
|
Test build #39589 has finished for PR 7902 at commit
|
Previous code assumed little-endian. Author: Matthew Brandyberry <[email protected]> Closes #7902 from mtbrandy/SPARK-9483 and squashes the following commits: ec31df8 [Matthew Brandyberry] [SPARK-9483] Changes from review comments. 17d54c6 [Matthew Brandyberry] [SPARK-9483] Fix UTF8String.getPrefix for big-endian. (cherry picked from commit b79b4f5) Signed-off-by: Davies Liu <[email protected]>
|
Merged into master and 1.5-branch, thanks! |
Previous code assumed little-endian.