GH-1179: Correct the size of var-width vector with >0 start offset during vector append#1180
GH-1179: Correct the size of var-width vector with >0 start offset during vector append#1180jordepic wants to merge 2 commits into
Conversation
jordepic
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
This comment has been minimized.
This comment has been minimized.
|
Could a maintainer please add the bug-fix label here? |
|
Hey @lidavidm , @laurentgo , @wgtmac , @jbonofre - would one of you guys mind taking a look here? It's a pretty nefarious bug :) |
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
LGTM.
I believe the same issue is present in ListVector and LargeListVector. Maybe worth to address in this PR or a following up PR.
|
@jbonofre would you be able to label the PR so that those checks aren't failing? I don't have permissions. I can also follow up here with the list vector changes. Thank you for your review :) |
|
@jordepic absolutely |
|
I'm not sure that the label worked if you tried to add it, I appreciate all of your help here |
jordepic
force-pushed
the
fix-vector-appender-nonzero-start-offset
branch
from
94537fa to
03491a1
Compare
|
@jbonofre I went ahead and amended this guy with the ListVector and LargeListVector fix. Changes are tested. Would greatly appreciate you labeling the PR as a bug fix as well as a review, thank you :) |
|
Thanks @lidavidm !! I think the last failure here is transient if you want to give it a reroll |
|
Go big red, by the way ;) |
|
@jbonofre if you want to do a final sweep all checks are passing! |
…width vectors with non-zero start offsets VectorAppender computed the delta vector's data size as its last offset value, which is only correct when the offset buffer starts at zero. Vectors imported through the C data interface from sliced arrays can have a non-zero first offset; appending them copied the unreferenced data buffer prefix into the target, inflating it on every append until allocation eventually failed with OversizedAllocationException. Compute the data size as the distance between the first and last offsets, copy from the first offset, and rebase appended offsets accordingly. Fixes apache#1179.
…ero start offsets Apply the same fix to list vectors: compute the referenced child range from the distance between the first and last offsets, rebase appended offsets by the delta's start offset, and append only the referenced range of the delta's data vector (via splitAndTransfer when the range does not cover the whole vector). Also fix LargeListVector appending, which was previously untested and broken: the target vector was cast to ListVector (ClassCastException for an actual LargeListVector target), and the offset buffer copy used 4-byte ListVector offset arithmetic on 8-byte offsets.
jordepic
force-pushed
the
fix-vector-appender-nonzero-start-offset
branch
from
03491a1 to
9bfd257
Compare
3 participants
What's Changed
Fix VectorAppender data size computation for variable-width vectors with non-zero start offsets
When appending a variable width offset vector in DataFusion comet I was receiving exceptions
due to allocating too much memory. This is because Comet passes variable width arrays back
to Java where the initial offset vector entry is greater than 0. Prior to this change, arrow-java
determines how many bytes to copy by just looking at the last offset entry in the buffer,
completely disregarding the value of the first. If first = 100 and last = 200, Java will still
copy 200 bytes instead of 100. In this change we fix that.
Closes #1179