apache / apache/datafusion-comet
Add a C++ UDF example and a published C header for the Comet UDF ABI
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
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` 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.
Contributor guide
Research direction
Start with native/comet-udf-sdk/src/c_abi.rs and read #5250 first, since the issue says its ownership contract should be settled before documenting the ABI. Then inspect the existing Spark suite and build setup. Done means a published header whose layouts match, a C++ add_one smoke test on Linux and macOS, and documentation covering exception and release semantics.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, cpp, rust, spark
- Domain
- backend-api-design, build-system, documentation, testing
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100