andygrove commented on PR #4459: URL: https://github.com/apache/datafusion-comet/pull/4459#issuecomment-5217963005
Thanks @mbutrovich — this was a useful pass. Pushed 8b573d4 with the fixes; I've replied on each thread, so this is just the map. **Fixed here** - `validateLibrary` returns void. The JSON construct/parse cycle could only echo back the name it was given, so it and the `require` that checked it are gone, along with `listUdfs`, which had no caller. - `classifyNativeError` no longer matches a bare `"ABI"`. It could misreport a missing file as an ABI mismatch for any library under a directory named `ABI`, because every loader message interpolates the path. Each failure mode now has a test pinning it to its exception type. - Registry entry is published before the catalog stub, closing the window you spotted. - The `Mutex` comment was wrong and is corrected. Concurrent tasks *do* share one `ImportedCScalarUdf` via the process-wide cache, so the lock is load-bearing and it serializes every batch of a UDF in the process. - `scalar_args` documented as reserved and always NULL in ABI v1. - The registry singleton now carries the lifetime/bounds justification the contributor guide asks for. - Banner comments removed. - Tests: concurrent registration, plus the four error-classification cases. Suite is 50 passing, 1 ignored. **The one that turned into a bug** Your question about a name collision was the most valuable comment in the set. It isn't just a namespace clash — a Rust UDF silently answers calls to an ordinary Scala UDF registered under the same name, returning the wrong values with no warning. The test is in the suite as `ignore`d with the observed failure recorded. The obvious fix doesn't work (`functions.udf` wraps the closure, and the overload that preserved identity is gone in Spark 4); it needs registering our own builder in the session `FunctionRegistry`, which is a follow-up rather than a merge-blocker patch. **Deliberately not fixed, follow-ups to file** Session scoping for the registry; the name collision above; per-batch `init` (the `return_field` decode is free to drop, but switching the import type interacts with nested nullability and wants the type matrix re-run); the cache's write lock across `dlopen`, including the poisoning consequence. I'll also correct #5252's body — it repeats the "DataFusion serializes invocations anyway" rationale, and removing the lock on that basis would introduce a real data race. **One I pushed back on** `catch_unwind` around the ABI-version probe: a caller-side guard can't catch a panic from the user's library, because an unwind reaching an `extern "C"` boundary aborts in the callee's frame. That's why the SDK's guards are callee-side, and why the macro's `comet_udf_abi_version` (a const return) can't panic. Happy to add a comment there instead if you'd like it recorded. On the design skepticism: noted, and I think the three things you pushed on — the process-wide registry, the process-wide cache and the lock granularity that follows from it — are the same concern wearing three hats. I'd rather resolve those in follow-ups on top of an experimental feature than carry them unexamined, but say so if you'd rather see the scoping settled before this lands. -- 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]
