andygrove opened a new issue, #5254: URL: https://github.com/apache/datafusion-comet/issues/5254
Follow-up from review of #4459 ([thread](https://github.com/apache/datafusion-comet/pull/4459#discussion_r2601302447)). The C ABI today exposes exactly `function_name`, `new_impl`, `init`, `execute`, `get_last_error` and `release`. Anything else a kernel might want to tell the host, volatility, `Display`/`Debug` renderings, documentation, aliases, needs a new struct field and a `COMET_UDF_ABI_VERSION` bump, which breaks every existing cdylib. @paleolimbot suggested a generic property getter instead, of the shape they are already using for table providers, exec plans and expressions: ```rust pub get_property: Option< unsafe extern "C" fn( *mut CometCScalarKernelImpl, property: *const c_char, args: *const c_char, out: *mut FFI_ArrowArray, ) -> c_int, >, ``` New properties then become new strings rather than new ABI, and an older cdylib answers "unsupported" for a property it does not know. The awkward part, as noted in the thread, is returning a variable-length string: using an `FFI_ArrowArray` for it works and is what the suggestion does, but is arguably overkill. Adding the field itself is an ABI change, so it wants to land before anyone depends on the current layout, or ride along with another version bump. Volatility (#5249) is the first concrete consumer. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
