ARROW-1443: [Java] Fixed a small bug on ArrowBuf.setBytes with unsliced ByteBuffers #1022
Conversation
|
This seems like a quite serious bug, perhaps this code path is never exercised? Can you add a unit test for this? Also, would you mind opening a JIRA for the changelog and add it to the PR title? Thanks! |
gortiz
changed the title
|
You can bet it was a serious bug. I spent some time trying to catch it! But it was quite easy to fix. I have pushed 3 commits: The first one reverts the previous one, the next creates the test and the last reapplies the fix. You can checkout 41bcacc to verify it was happening ;) |
|
|
||
| import static org.junit.Assert.*; | ||
|
|
||
| public class ArrowBufTest { |
There was a problem hiding this comment.
Can you rename this to TestArrowBuf?
|
Thanks @gortiz for catching this. This wasn't covered by any test because the code path is only used by VarBinaryVector and there is no java unit test for VarBinaryVector. @gortiz can you give me write permission to your branch? I'd like to add a test for VarBinaryVector as well. (If you are willing to add the test it'd be awesome too!) |
|
@icexelloss I'm not sure if there is other ways to give you write permission, but I've added you and @wesm as collaborators on my fork. That should be enough |
|
@gortiz Thanks. Actually there is test for VarBinaryVector but doesn't cover this code path. I will add test in TestValueVector.java. |
|
@gortiz I pushed a change that adds more vector tests to your branch. Can you rename ArrowBuf test and add license to the file? Otherwise LGTM |
|
@gortiz Can you remove the old ArrowBufTest.java file? |
|
Otherwise LGTM |
|
Done. My bad! |
wesm
left a comment
wesm
left a comment
There was a problem hiding this comment.
+1. thanks @icexelloss for reviewing!
|
thank you @gortiz for your contribution! |
|
Happy to help! I hope my employer decides to integrate Arrow in our project, in which case I will be pushing to contribute with more PRs! |
…ed ByteBuffers There is a small bug on ArrowBuf at line 750. It says: `udle.setBytes(index + offset, buf);` But it should say: `udle.setBytes(index + offset, newBuf);` There is a wraparound: You can call the method with the already sliced buffer. Author: Gonzalo Ortiz <golthiryus@gmail.com> Author: Li Jin <ice.xelloss@gmail.com> Closes apache#1022 from gortiz/master and squashes the following commits: cdffbcc [Gonzalo Ortiz] Removed the old ArrowBufTest (which was renamed as TestArrowBuf) d8aa00f [Gonzalo Ortiz] Renamed to ArrowBufTest to TestArrowBuf 1968a98 [Gonzalo Ortiz] Added license header aa6c457 [Li Jin] Add more tests for NullableVarChar and NullableVarBinary e65e380 [Gonzalo Ortiz] ARROW-1443 Reapplying the bug fix 41bcacc [Gonzalo Ortiz] ARROW-1443 Added a test to verify the bug f70ceff [Gonzalo Ortiz] ARROW-1443 Reverted the previous commit to reproduce the bug cef014f [Gonzalo Ortiz] [Java] Fixed a small bug on ArrowBuf.setBytes with unsliced ByteBuffers
3 participants
There is a small bug on ArrowBuf at line 750. It says:
udle.setBytes(index + offset, buf);But it should say:
udle.setBytes(index + offset, newBuf);There is a wraparound: You can call the method with the already sliced buffer.