apache / apache/datafusion-comet
Add a forward-flexible property slot to the Comet UDF C ABI
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
Follow-up from review of #4459 ([thread](https://github.com/apache/datafusion-comet/pull/4459#discussion_r3712885235)).
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.
Contributor guide
Research direction
Start with the existing Comet UDF C ABI and compare the property getter shape used for table providers, exec plans, and expressions, following the review thread from #4459. Confirm how unsupported properties and variable-length string results should be represented, then account for the ABI versioning concern and the volatility consumer in #5249.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, rust
- Domain
- api, backend-api-design
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100