apache / apache/datafusion-comet
Make kernel ownership transfer explicit in 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_r3712222988)).
`comet_c_udf_list_v1` hands the host a `CometCScalarKernelList`, and the host moves each `CometCScalarKernel` out of the array with `ptr::read`, then writes a `Default` (all callbacks `None`) back into the slot so the list release does not double-free.
That is correct today, but the correctness depends on the host remembering to write the default back. As @paleolimbot put it, it "seems like it would be easy to forget to do".
The suggested shape is to make the move explicit, the way the Arrow C Data Interface does it: the taker sets the source `release` callback to NULL and takes responsibility for the struct, and the list release then drops whatever kernels are still valid. Same net effect, but a host that forgets leaks rather than double-frees, and the contract is stated in the ABI instead of in a comment on the host side.
A related question was raised on the same code ([thread](https://github.com/apache/datafusion-comet/pull/4459#discussion_r3712620456)): whether a failure mid-import leaks. Tracing it, it does not. Kernels already imported are owned by `udfs`, the one that failed is dropped with the `Box` passed to `ImportedCScalarUdf::try_new`, and the rest are freed by the `list` local going out of scope. A test that forces a mid-import failure would make that durable rather than incidental, and would naturally come with this change.
Note this touches the ABI struct contract, so it wants a `COMET_UDF_ABI_VERSION` bump if it lands after the ABI is being consumed by anyone.
Contributor guide
Research direction
Start at the comet_c_udf_list_v1 entry point and follow CometCScalarKernel ownership through ptr::read, list release, and ImportedCScalarUdf::try_new. Define the explicit transfer contract so the source release callback is cleared, and add a test that forces a mid-import failure and verifies cleanup. Confirm whether the ABI change requires bumping COMET_UDF_ABI_VERSION.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, rust
- Domain
- api, backend-api-design
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100