andygrove opened a new issue, #5250: URL: https://github.com/apache/datafusion-comet/issues/5250
Follow-up from review of #4459 ([thread](https://github.com/apache/datafusion-comet/pull/4459#discussion_r2601283996)). `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_r2601298871)): 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. -- 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]
