You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/02/09 16:50:10 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #3685: Cleanup FFI interface (#3684) (#3683)
tustvold opened a new pull request, #3685:
URL: https://github.com/apache/arrow-rs/pull/3685
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3684
Closes #3683
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
The current interface is very hard to use correctly, with it being incredibly easy to accidentally leak memory. This reworks the interface to avoid this, and make better use of Rust's ownership semantics.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101848286
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
+//! let array = Int32Array::from(ArrayData::try_from(array)?);
//!
//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
//! let array = arithmetic::add(&array, &array)?;
//!
//! // verify
//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! #
+//! # Ok(())
+//! # }
+//! ```
//!
-//! // Simulate if raw pointers are provided by consumer
-//! let array = make_array(Int32Array::from(vec![Some(1), None, Some(3)]).into_data());
-//!
-//! let out_array = Box::new(FFI_ArrowArray::empty());
-//! let out_schema = Box::new(FFI_ArrowSchema::empty());
-//! let out_array_ptr = Box::into_raw(out_array);
-//! let out_schema_ptr = Box::into_raw(out_schema);
-//!
-//! // export array into raw pointers from consumer
-//! unsafe { export_array_into_raw(array, out_array_ptr, out_schema_ptr)?; };
-//!
-//! // import it
-//! let array = unsafe { make_array_from_raw(out_array_ptr, out_schema_ptr)? };
-//!
-//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
-//! let array = arithmetic::add(&array, &array)?;
+//! Import from FFI
//!
-//! // verify
-//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! ```
+//! # use std::ptr::addr_of_mut;
+//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
+//! # use arrow_array::{ArrayRef, make_array};
+//! # use arrow_schema::ArrowError;
+//! #
+//! /// A foreign data container that can export to C Data interface
+//! struct ForeignArray {};
//!
-//! // (drop/release)
-//! unsafe {
-//! Box::from_raw(out_array_ptr);
-//! Box::from_raw(out_schema_ptr);
-//! Arc::from_raw(array_ptr);
-//! Arc::from_raw(schema_ptr);
+//! impl ForeignArray {
+//! /// Export to C Data interface
+//! fn export(&self, array: *mut FFI_ArrowArray, schema: *mut FFI_ArrowSchema) {
+//! // ...
+//! }
Review Comment:
I was modelling it after the export_to_c function found in python
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101757793
##########
arrow/src/ffi.rs:
##########
@@ -911,6 +928,7 @@ impl ArrowArray {
}
/// exports [ArrowArray] to the C Data Interface
+ #[deprecated(note = "Use FFI_ArrowArray and FFI_ArrowSchema directly")]
Review Comment:
The semantics of this method are really unclear, it is never documented that the returned pointers need to be fed to `Arc::from_raw` in order to free them
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101758373
##########
arrow/src/ffi.rs:
##########
@@ -878,6 +894,7 @@ impl ArrowArray {
/// on managing the allocation of the structs by themselves.
/// # Error
/// Errors if any of the pointers is null
+ #[deprecated(note = "Use ArrowArray::new")]
Review Comment:
This method has a very misleading signature, it actually mutates the data pointed to by `array` and `schema`
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101840067
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
Because I wanted to reduce the number of methods that pass around raw pointers, and wanted to avoid making a breaking change. I can change it if you feel strongly, but having two methods one of which has to have a long explanation of its ownership semantics, as it does a sort of DIY indirect move, seemed like a code smell
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101831356
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
Yes, but the argument to `try_from_raw` is `*const` so there is a pretty reasonable assumption that it shouldn't mutate the provided data :sweat_smile:
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101847348
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
+//! let array = Int32Array::from(ArrayData::try_from(array)?);
//!
//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
//! let array = arithmetic::add(&array, &array)?;
//!
//! // verify
//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! #
+//! # Ok(())
+//! # }
+//! ```
//!
-//! // Simulate if raw pointers are provided by consumer
-//! let array = make_array(Int32Array::from(vec![Some(1), None, Some(3)]).into_data());
-//!
-//! let out_array = Box::new(FFI_ArrowArray::empty());
-//! let out_schema = Box::new(FFI_ArrowSchema::empty());
-//! let out_array_ptr = Box::into_raw(out_array);
-//! let out_schema_ptr = Box::into_raw(out_schema);
-//!
-//! // export array into raw pointers from consumer
-//! unsafe { export_array_into_raw(array, out_array_ptr, out_schema_ptr)?; };
-//!
-//! // import it
-//! let array = unsafe { make_array_from_raw(out_array_ptr, out_schema_ptr)? };
-//!
-//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
-//! let array = arithmetic::add(&array, &array)?;
+//! Import from FFI
//!
-//! // verify
-//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! ```
+//! # use std::ptr::addr_of_mut;
+//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
+//! # use arrow_array::{ArrayRef, make_array};
+//! # use arrow_schema::ArrowError;
+//! #
+//! /// A foreign data container that can export to C Data interface
+//! struct ForeignArray {};
//!
-//! // (drop/release)
-//! unsafe {
-//! Box::from_raw(out_array_ptr);
-//! Box::from_raw(out_schema_ptr);
-//! Arc::from_raw(array_ptr);
-//! Arc::from_raw(schema_ptr);
+//! impl ForeignArray {
+//! /// Export to C Data interface
+//! fn export(&self, array: *mut FFI_ArrowArray, schema: *mut FFI_ArrowSchema) {
+//! // ...
+//! }
Review Comment:
Import? I assume that idea is to import from foreign array into C Data Interface.
```suggestion
//! impl ForeignArray {
//! /// Import to C Data interface
//! fn import(&self, array: *mut FFI_ArrowArray, schema: *mut FFI_ArrowSchema) {
//! // ...
//! }
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101812613
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
Without `try_from_raw`, how do we import raw pointers from external into `ArrowArray`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101833169
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
> Isn't it jut like try_from_raw did now
Yes, this does not change what can be achieved. Instead it clarifies both the ownership and mutation by making the user be explicit about what they want. If they want to zero out the data, mutating it, they can use `std::ptr::replace` on a `*mut T`, if they just want to read the data, they can use `std::ptr::read`, if the pointer isn't aligned they can use `std::ptr::read_unaligned`, etc... The intention is to put the responsibility onto the caller as to what the pointer is and how to "correctly" read it
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101833169
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
> Isn't it jut like try_from_raw did now
Yes, this does not change what can be achieved. Instead it clarifies both the ownership and mutation by making the user be explicit about what they want. If they want to zero out the data, mutating it, they can use `std::ptr::replace` on a `*mut T`, if they just want to read the data, they can use `std::ptr::read`, if the pointer isn't aligned they can use `std::ptr::read_unaligned`. The intention is to put the responsibility onto the caller as to what the pointer is and how to "correctly" read it
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101824265
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
```
let array = std::ptr::replace(array_ptr, FFI_ArrowSchema::empty());
```
Perhaps?
Or depending on the semantics
```
let array = std::ptr::read(array_ptr);
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#issuecomment-1424822025
Benchmark runs are scheduled for baseline = 98ce68f3229a12e4e057802c4a2cd74170a66c73 and contender = bb4fc59009e7c5861a6b1967a53e9daa2554d5c6. bb4fc59009e7c5861a6b1967a53e9daa2554d5c6 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/4b0755bdc7084baea891eecdf1fdde02...5c55241f250d4bd089087a6c71d54af3/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e82acf77c4b0499cae50248c82e3964e...b568741083994f86a97568385d20e4c7/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/25890105d4b741dabea428bedf24d59b...2bc40dd925db4f898b8115e4cfd68d08/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5128d065d22640f79eed3b2c5339be32...1b139f4074bf4526b3964d1b95c2cd3c/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101814606
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
You either do something like in this doc - https://github.com/apache/arrow-rs/pull/3685/files#diff-c6cab492853dc6dec63fbf818294b78b87bdef628515910d91873084ff32d6d7R61
Or you use `std::ptr::replace`.
We could add something like `try_from_raw` that accepts `*mut` pointers, but I figured it was better to push people towards using Rust ownership instead of passing pointers around with unclear ownership
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101857293
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
I added a test using `std::ptr::read` as that is actually all that is necessary, assuming the foreign API is transferring ownership.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101822051
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
`array_ptr`, `schema_ptr` are somehow to simulate that the raw pointers from external (e.g. JVM in our usecase). Without `make_array_from_raw`, how can we import them as `ArrowArray`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101824265
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
```
let array = std::ptr::replace(array_ptr, FFI_ArrowSchema::empty());
```
Perhaps?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101814606
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
You either do something like in this doc - https://github.com/apache/arrow-rs/pull/3685/files#diff-c6cab492853dc6dec63fbf818294b78b87bdef628515910d91873084ff32d6d7R61
Or you use `std::ptr::replace`.
We could add something like `try_from_raw` that accepts `mut*` pointers, but I figured it was better to push people towards using Rust ownership instead of passing pointers around with unclear ownership
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101812613
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
Without `try_from_raw`, how do we import raw pointers into `ArrowArray`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101840067
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
Because I wanted to reduce the number of methods that pass around raw pointers, and wanted to avoid making a breaking change. I can change it if you feel strongly, but having two methods one of which has to have a long explanation of its ownership semantics, as it does a sort of DIY indirect aligned move, seemed like a code smell
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101840067
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
Because I wanted to reduce the number of methods that pass around raw pointers, and wanted to avoid making a breaking change. I can change it if you feel strongly, but having two methods one of which has to have a long explanation of its ownership semantics, as it does a sort of DIY pointer move, seemed like a code smell
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101840067
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
Because I wanted to reduce the number of methods that pass around raw pointers, and wanted to avoid making a breaking change
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101845334
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
Okay. Sounds good to me.
For the tests which use `try_from_raw`, could you try to make them use `std::ptr::replace` to import from raw pointers? This is the one of purposes of the tests to test importing from raw pointers as `ArrowArray`.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101831105
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
Hmm, do you mean to use `std::ptr::replace` in `ForeignArray.export`? Isn't it jut like `try_from_raw` did now?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101832068
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
BTW, I mean import from external pointers, not export.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101814606
##########
arrow/src/ffi.rs:
##########
@@ -1424,31 +1443,28 @@ mod tests {
let array = make_array(Int32Array::from(vec![1, 2, 3]).into_data());
// Assume two raw pointers provided by the consumer
- let out_array = Box::new(FFI_ArrowArray::empty());
- let out_schema = Box::new(FFI_ArrowSchema::empty());
- let out_array_ptr = Box::into_raw(out_array);
- let out_schema_ptr = Box::into_raw(out_schema);
-
- unsafe {
- export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ let mut out_array = FFI_ArrowArray::empty();
+ let mut out_schema = FFI_ArrowSchema::empty();
+
+ {
+ let out_array_ptr = addr_of_mut!(out_array);
+ let out_schema_ptr = addr_of_mut!(out_schema);
+ unsafe {
+ export_array_into_raw(array, out_array_ptr, out_schema_ptr)?;
+ }
}
// (simulate consumer) import it
- unsafe {
- let array = ArrowArray::try_from_raw(out_array_ptr, out_schema_ptr).unwrap();
Review Comment:
You either do something like in this doc - https://github.com/apache/arrow-rs/pull/3685/files#diff-c6cab492853dc6dec63fbf818294b78b87bdef628515910d91873084ff32d6d7R61
Or you use `std::ptr::replace`
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101837106
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
I see. How about make them as `*mut`? It sounds like we just remove `std::ptr::replace` usage and leave it to users like us. Although it is straightforward to write a importer function in our codebase, just wondering why we don't have it in upstream like this.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101828542
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
Review Comment:
`std::ptr::replace `? Isn't it something we did in `try_from_raw` nowadays?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101863374
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
+//! let array = Int32Array::from(ArrayData::try_from(array)?);
//!
//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
//! let array = arithmetic::add(&array, &array)?;
//!
//! // verify
//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! #
+//! # Ok(())
+//! # }
+//! ```
//!
-//! // Simulate if raw pointers are provided by consumer
-//! let array = make_array(Int32Array::from(vec![Some(1), None, Some(3)]).into_data());
-//!
-//! let out_array = Box::new(FFI_ArrowArray::empty());
-//! let out_schema = Box::new(FFI_ArrowSchema::empty());
-//! let out_array_ptr = Box::into_raw(out_array);
-//! let out_schema_ptr = Box::into_raw(out_schema);
-//!
-//! // export array into raw pointers from consumer
-//! unsafe { export_array_into_raw(array, out_array_ptr, out_schema_ptr)?; };
-//!
-//! // import it
-//! let array = unsafe { make_array_from_raw(out_array_ptr, out_schema_ptr)? };
-//!
-//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
-//! let array = arithmetic::add(&array, &array)?;
+//! Import from FFI
//!
-//! // verify
-//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! ```
+//! # use std::ptr::addr_of_mut;
+//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
+//! # use arrow_array::{ArrayRef, make_array};
+//! # use arrow_schema::ArrowError;
+//! #
+//! /// A foreign data container that can export to C Data interface
+//! struct ForeignArray {};
//!
-//! // (drop/release)
-//! unsafe {
-//! Box::from_raw(out_array_ptr);
-//! Box::from_raw(out_schema_ptr);
-//! Arc::from_raw(array_ptr);
-//! Arc::from_raw(schema_ptr);
+//! impl ForeignArray {
+//! /// Export to C Data interface
+//! fn export(&self, array: *mut FFI_ArrowArray, schema: *mut FFI_ArrowSchema) {
+//! // ...
+//! }
Review Comment:
Tweaked it to hopefully be a bit clearer, PTAL
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #3685: Cleanup FFI interface (#3684) (#3683)
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #3685:
URL: https://github.com/apache/arrow-rs/pull/3685#discussion_r1101866259
##########
arrow/src/ffi.rs:
##########
@@ -24,68 +24,61 @@
//! The second interface maps native Rust types to the Rust-specific implementation of Arrow such as `format` to `Datatype`,
//! `Buffer`, etc. This is handled by `ArrowArray`.
//!
+//!
+//! Export to FFI
+//!
//! ```rust
//! # use std::sync::Arc;
-//! # use arrow::array::{Int32Array, Array, ArrayData, export_array_into_raw, make_array, make_array_from_raw};
-//! # use arrow::error::{Result, ArrowError};
+//! # use arrow::array::{Int32Array, Array, ArrayData, make_array};
+//! # use arrow::error::Result;
//! # use arrow::compute::kernels::arithmetic;
//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
-//! # use std::convert::TryFrom;
//! # fn main() -> Result<()> {
//! // create an array natively
//! let array = Int32Array::from(vec![Some(1), None, Some(3)]);
+//! let data = array.into_data();
//!
-//! // export it
-//!
-//! let ffi_array = ArrowArray::try_new(array.data().clone())?;
-//! let (array_ptr, schema_ptr) = ArrowArray::into_raw(ffi_array);
-//!
-//! // consumed and used by something else...
+//! // Export it
+//! let out_array = FFI_ArrowArray::new(&data);
+//! let out_schema = FFI_ArrowSchema::try_from(data.data_type())?;
//!
//! // import it
-//! let array = unsafe { make_array_from_raw(array_ptr, schema_ptr)? };
+//! let array = ArrowArray::new(out_array, out_schema);
+//! let array = Int32Array::from(ArrayData::try_from(array)?);
//!
//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
//! let array = arithmetic::add(&array, &array)?;
//!
//! // verify
//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! #
+//! # Ok(())
+//! # }
+//! ```
//!
-//! // Simulate if raw pointers are provided by consumer
-//! let array = make_array(Int32Array::from(vec![Some(1), None, Some(3)]).into_data());
-//!
-//! let out_array = Box::new(FFI_ArrowArray::empty());
-//! let out_schema = Box::new(FFI_ArrowSchema::empty());
-//! let out_array_ptr = Box::into_raw(out_array);
-//! let out_schema_ptr = Box::into_raw(out_schema);
-//!
-//! // export array into raw pointers from consumer
-//! unsafe { export_array_into_raw(array, out_array_ptr, out_schema_ptr)?; };
-//!
-//! // import it
-//! let array = unsafe { make_array_from_raw(out_array_ptr, out_schema_ptr)? };
-//!
-//! // perform some operation
-//! let array = array.as_any().downcast_ref::<Int32Array>().ok_or(
-//! ArrowError::ParseError("Expects an int32".to_string()),
-//! )?;
-//! let array = arithmetic::add(&array, &array)?;
+//! Import from FFI
//!
-//! // verify
-//! assert_eq!(array, Int32Array::from(vec![Some(2), None, Some(6)]));
+//! ```
+//! # use std::ptr::addr_of_mut;
+//! # use arrow::ffi::{ArrowArray, FFI_ArrowArray, FFI_ArrowSchema};
+//! # use arrow_array::{ArrayRef, make_array};
+//! # use arrow_schema::ArrowError;
+//! #
+//! /// A foreign data container that can export to C Data interface
+//! struct ForeignArray {};
//!
-//! // (drop/release)
-//! unsafe {
-//! Box::from_raw(out_array_ptr);
-//! Box::from_raw(out_schema_ptr);
-//! Arc::from_raw(array_ptr);
-//! Arc::from_raw(schema_ptr);
+//! impl ForeignArray {
+//! /// Export to C Data interface
+//! fn export(&self, array: *mut FFI_ArrowArray, schema: *mut FFI_ArrowSchema) {
+//! // ...
+//! }
Review Comment:
Looks good to me. Thanks.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org