Repository navigation
feat: Add ScalarUDF support in FFI crate - #14579
Conversation
… forth between result and rresult
…t this shouldn't be necessary
…lean, but this shouldn't be necessary" This reverts commit 10248c2.
b13fcfe to
72279df
Compare
alamb
left a comment
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.
| #[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?
alamb
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks @timsaucer
|
|
||
| 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! |
…che#26423) ## Which issue does this PR close? This follows the same rationale as earlier PRs like - apache#10588 - apache#11831 - apache#18226 - apache#26419 ## Rationale for this change Each `.rs` file directly in `datafusion/ffi/tests` becomes its own test binary. In this case, the 10 standalone `ffi_*` binaries require about **2.48 GB** (2,664,138,160 bytes) in total on my machine (macOS, aarch64) ```shell cargo test -p datafusion-ffi --features integration-tests --tests --no-run ``` The binaries were added in - `ffi_integration`: apache#14579 - `ffi_udaf`: apache#14775 - `ffi_udf`: apache#15487 - `ffi_udtf`: apache#15581 - `ffi_udwf`: apache#16261 - `ffi_catalog`: apache#18657 - `ffi_config`: apache#19469 - `ffi_execution_plan`: apache#20449 - `ffi_physical_optimizer`: apache#20451 - `ffi_query_planner`: apache#24028 ## What changes are included in this PR? Combine the 10 `datafusion/ffi/tests/ffi_*.rs` files into the single `ffi_integration` test binary: Size of the binaries (debug build): | binary | bytes | |---|---| | `ffi_catalog` | 382,295,824 | | `ffi_config` | 6,295,496 | | `ffi_execution_plan` | 71,841,312 | | `ffi_integration` (before) | 382,373,904 | | `ffi_physical_optimizer` | 71,361,096 | | `ffi_query_planner` | 382,445,168 | | `ffi_udaf` | 320,780,232 | | `ffi_udf` | 319,111,528 | | `ffi_udtf` | 407,588,120 | | `ffi_udwf` | 320,045,480 | | **total before (10 binaries)** | **2,664,138,160** | | **`ffi_integration` after (1 binary)** | **408,453,600** | So this goes from about 2.48 GB to about 390 MB (408,453,600 bytes) (~85% smaller) and from 10 link steps to 1. ## What is the testing strategy for this PR? CI. The same 32 tests run before and after: ```shell cargo test -p datafusion-ffi --features integration-tests --test ffi_integration ``` ## Are there any user-facing changes? No, developer productivity / CI efficiency Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
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.