GH-470: ListViewVector#getElementEndIndex should returns size not index #554
Conversation
joyCurry30
requested review from
lidavidm,
laurentgo and
wgtmac
as code owners
kou
changed the title
kou
changed the title
| @@ -956,7 +956,7 @@ public int getElementStartIndex(int index) { | |||
|
|
|||
| @Override | |||
| public int getElementEndIndex(int index) { | |||
| return sizeBuffer.getInt(index * OFFSET_WIDTH); | |||
| return offsetBuffer.getInt((index + 1) * OFFSET_WIDTH); | |||
There was a problem hiding this comment.
Should line 954 be changed together? BTW, are these methods covered by any unit test?
There was a problem hiding this comment.
I don't see any unit tests for it; and if possible would be worth a small one.
There was a problem hiding this comment.
Yeah, the fact that tests passed means nothing's covering it. Unfortunately the new types were only partially implemented before the contributor left.
There was a problem hiding this comment.
Could add a few basic assertions to testBasicListViewVector https://github.com/apache/arrow-java/blob/main/vector/src/test/java/org/apache/arrow/vector/TestListViewVector.java
There was a problem hiding this comment.
Good suggest. I will add UT for it.
There was a problem hiding this comment.
Shouldn't the end index be offset + size + 1? (Assuming noninclusive)
|
@joyCurry30 Thank you so much for taking care of this! |
This comment has been minimized.
This comment has been minimized.
|
I am sorry for silence for so long. I have just add the ut for ListViewVector#getElementEndIndex. Could you please take a look when you are free? I'd be happy to address any feedback or make further improvement if needed. @nbauernfeind @wgtmac |
Close #470