Jefffrey commented on code in PR #10764:
URL: https://github.com/apache/arrow-rs/pull/10764#discussion_r3818128103
##########
arrow-schema/src/ffi.rs:
##########
@@ -194,12 +194,25 @@ impl FFI_ArrowSchema {
Ok(self)
}
- /// Add metadata to the schema
- pub fn with_metadata<I, S>(mut self, metadata: I) -> Result<Self,
ArrowError>
+ /// Add metadata to the schema.
+ ///
+ /// # Safety
+ ///
+ /// Reinterprets `private_data` as a value this crate produced. `self` must
+ /// come from arrow-rs (e.g. [`FFI_ArrowSchema::try_new`] or a `TryFrom`),
+ /// not a foreign producer. See
<https://github.com/apache/arrow-rs/issues/10679>.
+ pub unsafe fn with_metadata<I, S>(mut self, metadata: I) -> Result<Self,
ArrowError>
where
I: IntoIterator<Item = (S, S)>,
S: AsRef<str>,
{
+ // empty() leaves private_data null; error instead of deref-ing it
(#10286).
+ if self.private_data.is_null() {
Review Comment:
im on similar mind to this comment from @alamb
> Perhaps it would be more future proof to just set `private_data` for the
result of FFI_ArrowSchema::empty()?
>
> It seems like it is not unreasonable to set metadata on an empty schema,
and I worry that other existing (or newly added) code paths will assume
private_data is non null
- https://github.com/apache/arrow-rs/pull/10289#pullrequestreview-4672589156
that or set it here if we're adding metadata to an empty schema 🤔
though we can check the other ffi structs to see how they deal with 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
