andygrove commented on issue #5252: URL: https://github.com/apache/datafusion-comet/issues/5252#issuecomment-5218594456
Correcting the rationale in the description, per @mbutrovich in [review of #4459](https://github.com/apache/datafusion-comet/pull/4459#discussion_r3730396184). The description says the lock is "defensive rather than required", on the grounds that DataFusion serializes invocations of a given `ScalarUDFImpl` anyway. That is wrong. DataFusion does not serialize anything here — `ScalarUDFImpl: Send + Sync` exists so it does not have to — and `planner.rs` builds every task's `ScalarFunctionExpr` with `ScalarUDF::new_from_shared_impl` over the `Arc` from the process-wide cache in `cache.rs`. Concurrent Spark tasks in an executor are separate threads sharing one `ImportedCScalarUdf`, so concurrent `invoke_with_args` on it is the normal case, not an unusual one. **Simply removing the lock would introduce a data race.** What the lock actually stands in for is an ABI guarantee that does not exist yet. Rust's aliasing rules would not require it, since the callbacks reached through it (`function_name`, `new_impl`) take `*const CometCScalarKernel` and per-batch mutable state lives in the `CometCScalarKernelImpl` each call builds for itself. But ABI v1 does not require a kernel's `new_impl` to be callable concurrently, and the kernel is arbitrary user code. So this needs one of: - **Require thread-safe callbacks in the ABI**, document it, and hold the kernel behind a shared reference instead of a mutex — `Arc<CometCScalarKernel>` as @paleolimbot suggested, or plain `Box` given the struct is already `Send + Sync`. - **Hold a kernel per task** rather than one per process, so there is no sharing to protect. The consequence today, which is also worth stating in this issue: all batches of a given Rust UDF serialize through one mutex per executor process, so a Rust UDF over a wide scan does not scale with task parallelism. The comment on the `kernel` field in `imported_c.rs` has been corrected in #4459 and now describes the above. -- 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]
