ARROW-9861: [Java] Support big-endian in DecimalVector#8056
Conversation
kiszk
changed the title
|
@kiszk you'll probably need to make some fixes for Decimal256 as well, I think. @BryanCutler you mentioned you could devote some time to reviewing Big Endian changes? Would you mind taking a look through this one and @kiszk other Java changes? |
|
Sure, I can take a look. It might a day or two before I can though @kiszk . |
|
@emkornfield Sure, I will work for supporting Decimal256. |
BryanCutler
left a comment
BryanCutler
left a comment
There was a problem hiding this comment.
LGTM, just a couple minor things
There was a problem hiding this comment.
This seems a bit odd that it's writing long values at 2 different indices, but I guess that was here before. Do you know what it's trying to do here?
There was a problem hiding this comment.
Hmm, I guess it's to write a long value to be used as BigDecimal? So it writes the long in 8-bytes and then pads the remaining 8-bytes. It would be nice if the doc was a little better, but no big deal..
There was a problem hiding this comment.
This is used for DecialVector that is a fixed-width (16-byte) vector. This routine extends the signed bit to a new long. I will write a document in the comment.
There was a problem hiding this comment.
In addition, this PR will support to write it to a 256-bit entry for the future.
There was a problem hiding this comment.
why not return if length == TYPE_WIDTH?
There was a problem hiding this comment.
If we add if (length == TYPE_WIDTH) return before line 244, no data is copied from value to outAddress. I will add a comment copy data from value to outAddress after line 244.
There was a problem hiding this comment.
ok, I see above it is already set during the swap. I guess there is no harm in calling PlatformDependent.setMemory(outAddress, DecimalVector.TYPE_WIDTH - length, pad) if length == TYPE_WIDTH
There was a problem hiding this comment.
This should probably be in a Preconditions.checkArgument, but we are not trying to change things in this PR so don't need to do that here
There was a problem hiding this comment.
you could move padBytes out of the if statements
There was a problem hiding this comment.
I don't think you need 2 loops here, just move the Integer.MAX_VALUE and MIN_VALUE here
There was a problem hiding this comment.
Sure, I will drop lines 49-57.
|
I will update the Decimal256Vector class late today or tomorrow. |
BryanCutler
left a comment
BryanCutler
left a comment
There was a problem hiding this comment.
Looks good, just a few minor nits
There was a problem hiding this comment.
ok, I see above it is already set during the swap. I guess there is no harm in calling PlatformDependent.setMemory(outAddress, DecimalVector.TYPE_WIDTH - length, pad) if length == TYPE_WIDTH
There was a problem hiding this comment.
typo: DeciimalUtility -> DecimalUtility
There was a problem hiding this comment.
Could you use DecimalVector.TYPE_WIDTH here?
BryanCutler
left a comment
BryanCutler
left a comment
There was a problem hiding this comment.
LGTM. Looks like a flaky test, will try again.
|
The test error with Java JNI looks unrelated and seems to be an env issue with ORC, I'll go ahead with merging this. |
|
merged to master, thanks @kiszk ! |
|
@BryanCutler Thank you. One comment.
I am working for benchmark bot for Java here. It would be good to merge new features after this bot will be available. cc @emkornfield |
|
I did not see anything that looks like it would affect performance here, but I agree we should get some benchmarks going to be sure. I will look at you other PR next. |
This PR fixes failures in DecimalVectorTest on a big-endian platform Closes apache#8056 from kiszk/ARROW-9861 Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Signed-off-by: Bryan Cutler <cutlerb@gmail.com>
3 participants
This PR fixes failures in DecimalVectorTest on a big-endian platform