andygrove commented on PR #4459: URL: https://github.com/apache/datafusion-comet/pull/4459#issuecomment-5219196332
Following the observation that this isn't really Rust-specific — the ABI is `#[repr(C)]` structs of function pointers carrying Arrow C Data Interface arrays, unmangled C symbols, and library-supplied `release` callbacks, so nothing in the host cares what language produced the library — I've renamed the mechanism while it's still cheap (4cfd111): - proto `RustUdfCall` → **`NativeScalarUdf`**, field `rust_udf_call` → `native_scalar_udf` (field number 75 unchanged). This is the one I most wanted to fix before it lands: it parallels the existing `JvmScalarUdf`, and a wire name is the awkward one to change later. - `CometRustUDF` → **`CometNativeUDF`**, with the registry, bridge, metadata, exceptions and suite alongside it. - Rust internals to `execution::c_udf`, matching the C-ABI names already there (`CometCScalarKernel`, `comet_c_udf_list_v1`). - Messages and comments say "native UDF" for the mechanism, "Rust" for the SDK. `comet-udf-sdk`, `comet-test-udfs` and `rust_udfs.md` keep their names: the SDK genuinely is the Rust SDK, and a `cpp_udfs.md` would sit beside that page rather than replace it. I also narrowed a claim that went with the old naming. The guide said the ABI "is implementable from C or C++", which reads as an offer — true of the design, but nothing supports it: no header is published, the struct layouts live only in Rust source and aren't stable across releases, and the SDK's panic guards (which are what keep a UDF bug from aborting the executor) have no automatic equivalent in another language. #5301 tracks making it real — header, C++ example, CI smoke test — and notes that #5250 should land first, since the kernel-list release contract is currently implicit in a way only a non-Rust implementer would trip over. Drive-by while in the proto: the `NativeScalarUdf` comment still described dispatch through "whichever ABI flavor (C ABI / datafusion-ffi)" the library registered under, which has been stale since the datafusion-ffi flavor was dropped. `CometNativeUdfSuite` is 50 passing / 1 ignored; `cargo test`, `clippy`, `fmt`, spotless and scalastyle all clean. CI workflow references updated to the new suite name. -- 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]
