feat: enable metadata import/export through C data interface #3944
Conversation
|
I plan to review this carefully tomorrow |
arrow-schema/src/ffi.rs
Outdated
| let key_len = key.len() as i32; | ||
| let value_len = value.len() as i32; |
There was a problem hiding this comment.
Is it worth checking this doesn't overflow?
There was a problem hiding this comment.
I can't imagine this will ever come up as an issue. I'm also inferring from the fact that there's no conversion trait from the TryFrom associated error to ArrowError this isn't commonly handled elsewhere in the codebase.
There was a problem hiding this comment.
The codebase is full of overflow handling, take a look at the builders or take kernels or Array validation.
I'm keen that we are defensive here, because the implications of incorrect data is that we or the downstream reader go off reading arbitrary memory
| Ok(HashMap::new()) | ||
| } else { | ||
| let mut pos = 0; | ||
| let buffer: *const u8 = self.metadata as *const u8; |
There was a problem hiding this comment.
Possibly not much we can do about this, but it occurs to me that this parsing code can very easily read into arbitrary memory
There was a problem hiding this comment.
The one thing I can think of is we can verify the lengths are all >= 0. It doesn't guarantee we will detect buffer overflow but it will let us detect it some of the time 🤷
Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
|
Thank you |
Which issue does this PR close?
Closes #478.
Rationale for this change
Metadata is used in extension types to communicate additional information. For example, the geoarrow specification uses field metadata to communicate extension types and coordinate systems for geospatial arrays.
What changes are included in this PR?
Adds metadata round tripping for fields and schemas.
Are there any user-facing changes?
Fields and schemas brought through the C data interface will now have their metadata preserved.