feat: Add ScalarUDF support in FFI crate #14579
Conversation
github-actions
bot
added
development-process
… forth between result and rresult
…t this shouldn't be necessary
…lean, but this shouldn't be necessary" This reverts commit 10248c2.
timsaucer
force-pushed
the
feat/scalar-udf-ffi
branch
from
b13fcfe to
72279df
Compare
timsaucer
changed the title
timsaucer
changed the title
There was a problem hiding this comment.
Thank you @timsaucer - this looks good to me. I do think it would be valuable to seriously consider simplifying the Signature portion of the FFI to keep it smaller and more likely to be be stable
Also as I understand, having an FFI API for functions would make it easier for function libraries to be used without having to exactly match the DataFuson version which seems like a great idea to me.
datafusion/ffi/src/signature.rs
Outdated
| #[repr(C)] | ||
| #[derive(StableAbi)] | ||
| #[allow(non_camel_case_types)] | ||
| pub struct FFI_Signature { |
There was a problem hiding this comment.
I wonder if we should keep this API simpler as I think the actual signature API is about to change in the near future. For example, the work from @jayzhan211 and @shehabgamin in the following PR
One thought I had was to to only support the equivalent of TypeSignature::UserDefined
While this would make it somewhat less conveniente to implement user defined functions I think it would result in much smaller FFI API and also be much less likely to change overtime
Maybe once the TypeSignature / Coecion code has settled down we could add more to the FFI interface
There was a problem hiding this comment.
Does this now match what you were looking for?
|
|
||
| pub volatility: FFI_Volatility, | ||
|
|
||
| pub return_type: unsafe extern "C" fn( |
There was a problem hiding this comment.
Eventually it would be great to document these a bit more -- for example here we can mention that we only expose the return type method to simplify the signature
|
woohoo! |
Which issue does this PR close?
Addresses part of #14562 - specifically scalar udfs
Rationale for this change
This is a pure addition were we expose
ScalarUDFacross the FFI boundary. This enables new functionality about creating reusable libraries, such as in python.What changes are included in this PR?
Addition of scalar udf FFI wrappers
Are these changes tested?
Are there any user-facing changes?
This is a pure addition.