andygrove opened a new issue, #5301: URL: https://github.com/apache/datafusion-comet/issues/5301
Follow-up from #4459 ([discussion](https://github.com/apache/datafusion-comet/pull/4459#issuecomment-5217963005)). ## Why The UDF ABI in #4459 is not Rust-specific by design, and by construction: - `CometCScalarKernel`, `CometCScalarKernelImpl` and `CometCScalarKernelList` are all `#[repr(C)]` and contain only function pointers, raw pointers and an `i64`. `Option<unsafe extern "C" fn(..)>` is a plain function pointer in C thanks to the null-pointer optimization. - The only payload types crossing the boundary are `FFI_ArrowSchema` / `FFI_ArrowArray`, which are `#[repr(C)]` renderings of the spec-defined [Arrow C Data Interface](https://arrow.apache.org/docs/format/CDataInterface.html). - The two entry points are unmangled C symbols: `comet_udf_abi_version` and `comet_c_udf_list_v1`. - Every allocation is freed through a `release` callback the library supplies, so there is no assumption that the library uses Rust's allocator. The proto message is named `NativeScalarUdf` rather than `RustUdfCall` for this reason. But "a C++ UDF would work" is currently a claim with nothing behind it, and the user guide has been narrowed to say so. ## What would make it real **1. Publish a C header.** Today a C or C++ author has to hand-transcribe three structs and their lifecycle contracts out of `native/comet-udf-sdk/src/c_abi.rs`. That is worse than it sounds, because the layouts are explicitly *not* ABI stable across Comet releases, so the transcription has to be redone and re-verified on every upgrade — with only `comet_udf_abi_version` as a backstop. Either generate `comet_udf.h` with cbindgen as part of the build, or hand-write it and have a test assert the layouts agree. **2. Ship an example plus a CI smoke test.** An untested language claim decays immediately. A minimal `add_one` in C++, built in CI on Linux and macOS and driven through the existing Spark suite, is what keeps it honest. **3. Document what the SDK does for Rust authors that a C++ author must do themselves.** Two things at least: - **Exception containment.** The SDK's `catch_panic` / `catch_panic_infallible` wrap every `extern "C"` entry point, which is what keeps a bug in user code from aborting the executor JVM and losing every task on it. A C++ author gets none of that: an exception escaping `extern "C"` terminates the process just as an escaping Rust panic would. Every entry point needs its own `try`/`catch` translating to a non-zero return plus a `get_last_error` message. - **Kernel-list release semantics.** `read_c_kernels` moves each kernel out of the array with `ptr::read`, writes an all-null `CometCScalarKernel` back into the slot, and only then lets the list's `release` run. So a `release` implementation must tolerate zeroed entries: it may not iterate the array freeing `private_data` or calling each kernel's `release`. Rust authors never meet this because `build_kernel_list` and its release callback are generated by `comet_c_udf_export!`. This overlaps #5250, which is about making that ownership transfer explicit in the ABI — worth doing first, since the header would otherwise document an implicit contract. ## Not in scope Adding a *supported* C++ SDK (a header-only wrapper with RAII and exception translation, equivalent to what `comet-udf-sdk` gives Rust). Worth considering separately if there is demand; this issue is about the ABI being demonstrably usable from another language, and staying that way. -- 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]
