perf: Use Cow in get_format_string in FFI_ArrowSchema #6853
Conversation
| @@ -685,66 +686,68 @@ impl TryFrom<&DataType> for FFI_ArrowSchema { | |||
| } | |||
| } | |||
|
|
|||
| fn get_format_string(dtype: &DataType) -> Result<String, ArrowError> { | |||
| fn get_format_string(dtype: &DataType) -> Result<Cow<'static, str>, ArrowError> { | |||
There was a problem hiding this comment.
Looks like we don't need it to be owned. Maybe just &str?
There was a problem hiding this comment.
Are you saying we can use &str return instead of Cow? some of the data types use format! to create new strings so I don't think that can work?
There was a problem hiding this comment.
Oh, I see. I didn't see there are Cow::Owned.
|
It fails msrv check. Otherwise the performance looks good. |
There was a problem hiding this comment.
Thanks @andygrove and @viirya -- this looks good to me as well (modulo the test failures)
arrow-schema/src/ffi.rs
Outdated
| DataType::Struct(_) => Ok("+s".to_string()), | ||
| DataType::Map(_, _) => Ok("+m".to_string()), | ||
| DataType::RunEndEncoded(_, _) => Ok("+r".to_string()), | ||
| DataType::Null => Ok(Cow::Borrowed("n")), |
There was a problem hiding this comment.
This syntax is totally fine (and very explicit)
I think you can express the same thing using into which is less verbose:
For example
| DataType::Null => Ok(Cow::Borrowed("n")), | |
| DataType::Null => Ok("n".into()), |
There was a problem hiding this comment.
That is much nicer. Updated.
| @@ -685,66 +686,68 @@ impl TryFrom<&DataType> for FFI_ArrowSchema { | |||
| } | |||
| } | |||
|
|
|||
| fn get_format_string(dtype: &DataType) -> Result<String, ArrowError> { | |||
| fn get_format_string(dtype: &DataType) -> Result<Cow<'static, str>, ArrowError> { | |||
There was a problem hiding this comment.
strictly speaking this is an API change, but since the next scheduled release is a major one (allows API changes) we can merge it in to main without worry
There was a problem hiding this comment.
I don't think that this function is exposed as part of the public API for this crate?
Which issue does this PR close?
N/A
Rationale for this change
We're using a lot of FFI in Comet and are looking for any performance wins we can get.
What changes are included in this PR?
Use
Cowinget_format_stringto reduce some string allocations.Are there any user-facing changes?
No