apache / apache/datafusion-comet

Make kernel ownership transfer explicit in the Comet UDF C ABI

Open
#5,250 0 comments 0 reactions 0 assignees View on GitHub
enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.