GH-44626: [Java] fix SplitAndTransfer throws for empty MapVector #44627
Conversation
| final int relativeOffset = | ||
| offsetBuffer.getInt((startIndex + i) * OFFSET_WIDTH) - startPoint; | ||
| to.offsetBuffer.setInt(i * OFFSET_WIDTH, relativeOffset); | ||
| if (length > 0) { |
There was a problem hiding this comment.
can we just return if length=0
There was a problem hiding this comment.
Can do though my personal preference would be to avoid multiple returns. Current implementation is in keeping with how splitAndTransfer is implemented for other complex types (e.g. ListVector).
github-actions
bot
added
awaiting committer review
| ListVector toListVector = (ListVector) transferPair.getTo(); | ||
|
|
||
| assertEquals(valueCount, toListVector.getValueCount()); | ||
| fromListVector.clear(); |
There was a problem hiding this comment.
nit: use try-with-resources
There was a problem hiding this comment.
Note that there're several closeable resources here and toListVector cannot be constructed inside the same try as fromListVector. This is just to say that there'd be some ugly nesting if using try.
|
@lidavidm need some help to run the CIs. |
Signed-off-by: Maksim Yegorov <[email protected]>
| MapVector dataVector = (MapVector) fromListVector.getDataVector(); | ||
| dataVector.allocateNew(); | ||
| // unset capacity to mimic observed failure mode | ||
| dataVector.getOffsetBuffer().capacity(0); |
There was a problem hiding this comment.
Hmm, a list vector is supposed to have 1 offset when it's empty. But I suppose other parts of the library have this issue/"optimization"
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 5fe87a3. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Rationale for this change
Empty MapVector.splitAndTransfer throws
java.lang.IndexOutOfBoundsException. Details in #44626What changes are included in this PR?
Fixed for MapVector as for other vector types in #41066
Are these changes tested?
Added unit test mimicking the scenario we've observed where MapVector's offset buffer capacity is 0.