Skip to content
Open
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions arrow-schema/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's lead with

self must be a Schema created in arrow-rs

As that is the key point (the reinterprets... part of the comment is an implementation detail)

/// 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() {
Comment thread
alamb marked this conversation as resolved.
return Err(ArrowError::CDataInterface(
"Cannot add metadata to a schema with no private data".to_string(),
));
}

let metadata: Vec<(S, S)> = metadata.into_iter().collect();
// https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema.metadata
let new_metadata = if !metadata.is_empty() {
Expand Down Expand Up @@ -867,10 +880,11 @@ impl TryFrom<&Field> for FFI_ArrowSchema {
flags |= Flags::DICTIONARY_ORDERED;
}

FFI_ArrowSchema::try_from(field.data_type())?
let schema = FFI_ArrowSchema::try_from(field.data_type())?
.with_name(field.name())?
.with_flags(flags)?
.with_metadata(field.metadata())
.with_flags(flags)?;
// SAFETY: schema was just constructed by this crate.
unsafe { schema.with_metadata(field.metadata()) }
}
}

Expand All @@ -879,8 +893,9 @@ impl TryFrom<&Schema> for FFI_ArrowSchema {

fn try_from(schema: &Schema) -> Result<Self, ArrowError> {
let dtype = DataType::Struct(schema.fields().clone());
let c_schema = FFI_ArrowSchema::try_from(&dtype)?.with_metadata(&schema.metadata)?;
Ok(c_schema)
let c_schema = FFI_ArrowSchema::try_from(&dtype)?;
// SAFETY: c_schema was just constructed by this crate.
unsafe { c_schema.with_metadata(&schema.metadata) }
}
}

Expand Down Expand Up @@ -1065,12 +1080,21 @@ mod tests {
.unwrap();

for metadata in metadata_cases {
schema = schema.with_metadata(&metadata).unwrap();
// SAFETY: schema was constructed by this crate via try_new.
schema = unsafe { schema.with_metadata(&metadata) }.unwrap();
let field = Field::try_from(&schema).unwrap();
assert_eq!(field.metadata(), &metadata);
}
}

#[test]
fn test_with_metadata_on_empty_schema_errors() {
// empty() has null private_data; with_metadata errors instead of UB (#10286).
let schema = FFI_ArrowSchema::empty();
let result = unsafe { schema.with_metadata([("key", "value")]) };
assert!(result.is_err());
}

#[test]
fn test_name_with_null_byte() {
let schema = FFI_ArrowSchema::try_new("i", vec![], None).unwrap();
Expand Down
Loading