Pass ConfigOptions to scalar UDFs via FFI#20454
Conversation
github-actions
bot
added
common
timsaucer
force-pushed
the
feat/ffi-scalar-udf-config
branch
from
44f3a91 to
da8080d
Compare
timsaucer
changed the title
There was a problem hiding this comment.
Pull request overview
This PR enables passing ConfigOptions to scalar UDFs via the FFI (Foreign Function Interface), resolving issue #17035. The changes allow external FFI libraries to access session configuration options when executing scalar functions, which was previously blocked by the TODO comment about passing config options.
Changes:
- Added
config_optionsparameter to FFI scalar UDF invocation signature - Switched from returning
WrappedArraytoFFI_ColumnarValuefor better scalar/array handling - Added comprehensive test validating config option passing through FFI boundary
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| datafusion/ffi/tests/ffi_udf.rs | Added test test_config_on_scalar_udf validating that config options (specifically timezone) are correctly passed to FFI UDFs |
| datafusion/ffi/src/udf/mod.rs | Updated FFI scalar UDF signatures to include config_options parameter, converted between FFI and native ConfigOptions, switched to FFI_ColumnarValue return type |
| datafusion/ffi/src/tests/udf_udaf_udwf.rs | Implemented TimeZoneUDF test helper that returns the configured timezone |
| datafusion/ffi/src/tests/mod.rs | Added create_timezone_udf function to the foreign library module exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@alamb Asking for your review since you put up the issue, but I can try to find someone who's spent more time on the FFI code if you like |
davisp
left a comment
davisp
left a comment
There was a problem hiding this comment.
+1
It's a straightforward addition now that FFI_ConfigOptions exists.
| let config_options = rresult_return!(ConfigOptions::try_from(config_options)); | ||
| let config_options = Arc::new(config_options); | ||
|
|
||
| let args = ScalarFunctionArgs { | ||
| args, | ||
| arg_fields, | ||
| number_rows, | ||
| return_field, | ||
| // TODO: pass config options: https://github.com/apache/datafusion/issues/17035 | ||
| config_options: Arc::new(ConfigOptions::default()), | ||
| config_options, | ||
| }; | ||
|
|
||
| let result = rresult_return!( | ||
| rresult!( | ||
| udf.inner() | ||
| .invoke_with_args(args) | ||
| .and_then(|r| r.to_array(number_rows)) | ||
| ); | ||
|
|
||
| let (result_array, result_schema) = rresult_return!(to_ffi(&result.to_data())); | ||
|
|
||
| RResult::ROk(WrappedArray { | ||
| array: result_array, | ||
| schema: WrappedSchema(result_schema), | ||
| }) | ||
| .and_then(FFI_ColumnarValue::try_from) | ||
| ) | ||
| } |
There was a problem hiding this comment.
This is the only change of much significance in the PR. Everything else is plumbing or tests.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Not my most familiar part of the code, but what's here made sense to me. Thanks @timsaucer!
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_config_on_scalar_udf() -> Result<()> { |
|
Thanks @timsaucer and @mbutrovich and @davisp |
## Which issue does this PR close? - Closes apache#17035 ## Rationale for this change Now that we have proper `FFI_ConfigOptions` we can pass these to scalar UDFs via FFI. ## What changes are included in this PR? Instead of passing default options, pass in converted config options from the input. Also did a drive by cleanup of switching to using FFI_ColumnarValue since it is now available. ## Are these changes tested? Unit test added. ## Are there any user-facing changes? This is a breaking API change, but not one that users will interact with directly. It breaks the ABI for FFI libraries, which is currently unstable.
Which issue does this PR close?
ConfigOptionsto scalar functions via FFI #17035Rationale for this change
Now that we have proper
FFI_ConfigOptionswe can pass these to scalar UDFs via FFI.What changes are included in this PR?
Instead of passing default options, pass in converted config options from the input.
Also did a drive by cleanup of switching to using FFI_ColumnarValue since it is now available.
Are these changes tested?
Unit test added.
Are there any user-facing changes?
This is a breaking API change, but not one that users will interact with directly. It breaks the ABI for FFI libraries, which is currently unstable.