You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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 Make kernel ownership transfer explicit in the Comet UDF C ABI #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.
Follow-up from #4459 (discussion).
Why
The UDF ABI in #4459 is not Rust-specific by design, and by construction:
CometCScalarKernel,CometCScalarKernelImplandCometCScalarKernelListare all#[repr(C)]and contain only function pointers, raw pointers and ani64.Option<unsafe extern "C" fn(..)>is a plain function pointer in C thanks to the null-pointer optimization.FFI_ArrowSchema/FFI_ArrowArray, which are#[repr(C)]renderings of the spec-defined Arrow C Data Interface.comet_udf_abi_versionandcomet_c_udf_list_v1.releasecallback the library supplies, so there is no assumption that the library uses Rust's allocator.The proto message is named
NativeScalarUdfrather thanRustUdfCallfor 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 onlycomet_udf_abi_versionas a backstop. Either generatecomet_udf.hwith 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_onein 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:
catch_panic/catch_panic_infalliblewrap everyextern "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 escapingextern "C"terminates the process just as an escaping Rust panic would. Every entry point needs its owntry/catchtranslating to a non-zero return plus aget_last_errormessage.read_c_kernelsmoves each kernel out of the array withptr::read, writes an all-nullCometCScalarKernelback into the slot, and only then lets the list'sreleaserun. So areleaseimplementation must tolerate zeroed entries: it may not iterate the array freeingprivate_dataor calling each kernel'srelease. Rust authors never meet this becausebuild_kernel_listand its release callback are generated bycomet_c_udf_export!. This overlaps Make kernel ownership transfer explicit in the Comet UDF C ABI #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-sdkgives Rust). Worth considering separately if there is demand; this issue is about the ABI being demonstrably usable from another language, and staying that way.