jorgecarleitao commented on a change in pull request #8401:
URL: https://github.com/apache/arrow/pull/8401#discussion_r523763314
##########
File path: rust/arrow/src/buffer.rs
##########
@@ -151,70 +80,52 @@ impl Buffer {
///
/// * `ptr` - Pointer to raw parts
/// * `len` - Length of raw parts in **bytes**
- /// * `capacity` - Total allocated memory for the pointer `ptr`, in
**bytes**
+ /// * `data` - An [ffi::FFI_ArrowArray] with the data
///
/// # Safety
///
/// This function is unsafe as there is no guarantee that the given
pointer is valid for `len`
- /// bytes. If the `ptr` and `capacity` come from a `Buffer`, then this is
guaranteed.
- pub unsafe fn from_unowned(ptr: *const u8, len: usize, capacity: usize) ->
Self {
- Buffer::build_with_arguments(ptr, len, capacity, false)
+ /// bytes and that the foreign deallocator frees the region.
+ pub unsafe fn from_unowned(
+ ptr: *const u8,
+ len: usize,
+ data: Arc<ffi::FFI_ArrowArray>,
+ ) -> Self {
+ Buffer::build_with_arguments(ptr, len, Deallocation::Foreign(data))
}
- /// Creates a buffer from an existing memory region (must already be
byte-aligned).
- ///
- /// # Arguments
- ///
- /// * `ptr` - Pointer to raw parts
- /// * `len` - Length of raw parts in bytes
- /// * `capacity` - Total allocated memory for the pointer `ptr`, in
**bytes**
- /// * `owned` - Whether the raw parts is owned by this `Buffer`. If true,
this `Buffer` will
- /// free this memory when dropped, otherwise it will skip freeing the raw
parts.
- ///
- /// # Safety
- ///
- /// This function is unsafe as there is no guarantee that the given
pointer is valid for `len`
- /// bytes. If the `ptr` and `capacity` come from a `Buffer`, then this is
guaranteed.
+ /// Auxiliary method to create a new Buffer
unsafe fn build_with_arguments(
ptr: *const u8,
len: usize,
- capacity: usize,
- owned: bool,
+ deallocation: Deallocation,
) -> Self {
- assert!(
- memory::is_aligned(ptr, memory::ALIGNMENT),
Review comment:
Good question.
Memory alignment is not a requirement by the spec, it is a recommendation.
When we allocate in rust, the data is always allocated using
`memory::ALIGNMENT`. This constant currently depends on the architecture.
This assert risks that any data not allocated by Rust to panic, as there is
no guarantee of its alignment to `memory::ALIGNMENT` (i.e. other allocators may
as well use an architecture-independent alignment).
AFAIK, the only alignment that we really need is the one that allow us to
deref a pointer to a rust type (e.g. `i32`), and those checks are still
performed when such an operation is done (as long as people are not using very
unsafe code).
This was discussed some months ago on the mailing list, where it was
received with some surprise that Rust required an architecture-dependent
alignment. The issue here: https://issues.apache.org/jira/browse/ARROW-10039
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
