-
Notifications
You must be signed in to change notification settings - Fork 770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Adjust FFI_ArrowArray offset based on the offset of offset buffer #5895
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,24 @@ impl Buffer { | |
} | ||
} | ||
|
||
/// Returns the offset, in bytes, of `Self::ptr` to `Self::data` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
/// | ||
/// self.ptr and self.data can be different after slicing or advancing the buffer. | ||
/// | ||
/// # Safety | ||
/// | ||
/// This function is unsafe as it uses unsafe function `offset_from`. Under certain | ||
/// conditions, this function can cause undefined behavior. See the documentation of | ||
/// `offset_from` for more information. | ||
pub unsafe fn ptr_offset(&self) -> usize { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if this function should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
self.ptr.offset_from(self.data.ptr().as_ptr()) as usize | ||
} | ||
|
||
/// Returns the pointer to the start of the buffer without the offset. | ||
pub fn data_ptr(&self) -> NonNull<u8> { | ||
self.data.ptr() | ||
} | ||
|
||
/// Create a [`Buffer`] from the provided [`Vec`] without copying | ||
#[inline] | ||
pub fn from_vec<T: ArrowNativeType>(vec: Vec<T>) -> Self { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,6 +131,43 @@ impl FFI_ArrowArray { | |
data.buffers().iter().map(|b| Some(b.clone())).collect() | ||
}; | ||
|
||
// Handle buffer offset for offset buffer. | ||
let offset_offset = match data.data_type() { | ||
DataType::Utf8 | DataType::Binary => { | ||
// Offset buffer is possible a slice of the buffer. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this looks much nicer now |
||
// If we use slice pointer as exported buffer pointer, it will cause | ||
// the consumer to calculate incorrect length of data buffer (buffer 1). | ||
// We need to get the offset of the offset buffer and fill it in | ||
// the `FFI_ArrowArray` offset field. | ||
Some(unsafe { | ||
let b = &data.buffers()[0]; | ||
b.ptr_offset() / std::mem::size_of::<i32>() | ||
}) | ||
} | ||
DataType::LargeUtf8 | DataType::LargeBinary => { | ||
// Offset buffer is possible a slice of the buffer. | ||
// If we use slice pointer as exported buffer pointer, it will cause | ||
// the consumer to calculate incorrect length of data buffer (buffer 1). | ||
// We need to get the offset of the offset buffer and fill it in | ||
// the `FFI_ArrowArray` offset field. | ||
Some(unsafe { | ||
let b = &data.buffers()[0]; | ||
b.ptr_offset() / std::mem::size_of::<i64>() | ||
}) | ||
} | ||
_ => None, | ||
}; | ||
|
||
let offset = if let Some(offset) = offset_offset { | ||
if data.offset() != 0 { | ||
// TODO: Adjust for data offset | ||
panic!("The ArrayData of a slice offset buffer should not have offset"); | ||
} | ||
offset | ||
} else { | ||
data.offset() | ||
}; | ||
|
||
// `n_buffers` is the number of buffers by the spec. | ||
let n_buffers = { | ||
data_layout.buffers.len() + { | ||
|
@@ -143,9 +180,25 @@ impl FFI_ArrowArray { | |
|
||
let buffers_ptr = buffers | ||
.iter() | ||
.flat_map(|maybe_buffer| match maybe_buffer { | ||
// note that `raw_data` takes into account the buffer's offset | ||
Some(b) => Some(b.as_ptr() as *const c_void), | ||
.enumerate() | ||
.flat_map(|(buffer_idx, maybe_buffer)| match maybe_buffer { | ||
Some(b) => { | ||
match (data.data_type(), buffer_idx) { | ||
( | ||
DataType::Utf8 | ||
| DataType::LargeUtf8 | ||
| DataType::Binary | ||
| DataType::LargeBinary, | ||
1, | ||
) => { | ||
// For offset buffer, take original pointer without offset. | ||
// Buffer offset should be handled by `FFI_ArrowArray` offset field. | ||
Some(b.data_ptr().as_ptr() as *const c_void) | ||
} | ||
// For other buffers, note that `raw_data` takes into account the buffer's offset | ||
_ => Some(b.as_ptr() as *const c_void), | ||
} | ||
} | ||
// This is for null buffer. We only put a null pointer for | ||
// null buffer if by spec it can contain null mask. | ||
None if data_layout.can_contain_null_mask => Some(std::ptr::null()), | ||
|
@@ -186,7 +239,7 @@ impl FFI_ArrowArray { | |
Self { | ||
length: data.len() as i64, | ||
null_count: null_count as i64, | ||
offset: data.offset() as i64, | ||
offset: offset as i64, | ||
n_buffers, | ||
n_children, | ||
buffers: private_data.buffers_ptr.as_mut_ptr(), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this test without the code changes in this PR and it fails like
Thus I believe the test correctly covers the new code