alamb commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2582255380
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -452,7 +462,19 @@ mod tests {
let array = unsafe { from_ffi(ffi_array, &ffi_schema) }.unwrap();
- let record_batch = RecordBatch::from(StructArray::from(array));
+ let record_batch = {
Review Comment:
same here -- unless there is some compelling reason we shouldn't be using
`unsafe`, even thought this is test code
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array,
DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
> I'm not sure whether there is another more elegant way to construct a
RecordBatch with corresponding metadata from ArrayData. Right now I'm going
through StructArray because the previous interface did that too. If there is
another more elegant way, please let me know.
What is current on main is pretty elegant
```rust
RecordBatch::from(StructArray::from(data)))
```
It is very important that this crate doesn't introduce unsoundness bugs, so
in general we try and avoid `unsafe` unless there is a compelling
justification, as described here
https://github.com/apache/arrow-rs/blob/main/arrow/README.md#safety
So at the least this code should have a justification (as a comment) about
why `unsafe` is ok (aka why the invariants are known to be true) rather than
using the safe alternate
I suggest:
1. Remove the use of unsafe in this PR
2. Make a follow on PR that proposes converting this to `unsafe` where we
can discuss the merits in a focused discussion
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
