andygrove opened a new pull request, #6775: URL: https://github.com/apache/datafusion-comet/pull/6775
## Which issue does this PR close? Closes #6175. Closes #5297. ## Rationale for this change `CometNativeUDF.register` loads whatever shared library the caller names, on the driver and on every executor, with the full privileges of the JVM. An operator had no way to turn the feature off or to limit where libraries can come from (#6175). Separately, the native UDF library cache held its single write lock across `dlopen`, so one slow library load blocked lookups of every other library, and a panic under the lock poisoned it and disabled native UDFs for the life of the process (#5297). ## What changes are included in this PR? **Load policy (#6175)** - `spark.comet.nativeUdf.enabled` (default `true`): when `false`, no native UDF library is loaded. - `spark.comet.nativeUdf.allowedPaths` (default empty, meaning unrestricted): comma-separated directories a library must be under. The library path and each directory are canonicalized before comparing, so `..` and symlinks cannot escape, and matching is by path component (`/opt/udfs` does not match `/opt/udfs-other`). With a list set, a relative path or one that does not resolve is refused, since the dynamic loader's search path cannot be checked. - The policy lives in one Rust type (`c_udf/policy.rs`) used on both sides. The driver passes the two configs to `validateLibrary`, so `register` fails with the new `CometNativeUdfNotAllowedException`. The executor receives them in the serialized Spark configs, keeps them as a DataFusion session config extension, and the planner checks the path in the `NativeScalarUdf` before loading it, so a plan built elsewhere cannot bypass the driver check. - Defaults to enabled so existing opt-in-by-API-call behavior is unchanged. - Docs: new "Restricting which libraries can be loaded" section in `rust_udfs.md`, including that these are ordinary session configs and the allow-list is not protected against a replaced file. **Library cache (#5297)** - The cache is now a map of per-library slots. The map lock is only held for a lookup or insert, never across a load; each slot's mutex is held while that library loads, so concurrent requests for the same library share one load and requests for other libraries are not blocked. - Poisoned locks are recovered rather than unwrapped. A slot is only filled after a successful load, so a library whose load panics fails its own queries and the next request retries. - Libraries are still never unloaded, and a symlink and its target still share one `LoadedLibrary`. ## How are these changes tested? - Rust unit tests in `policy.rs` (disabled, allow-list match, sibling-prefix directory, `..` and symlink escapes, symlinked allowed directory, relative and missing paths, comma parsing, config parsing), in `cache.rs` (a blocked load does not block another library, concurrent requests load once, a panicking load does not disable the cache, a failed load is retried), and a planner test that the policy extension is applied to a `NativeScalarUdf`. - Four new cases in `CometNativeUdfSuite`: register refused when disabled, register refused outside `allowedPaths`, register and run under `allowedPaths`, and the executor refusing a function registered under the default policy once the policy is tightened. The suite passes (72 tests) on the default Spark profile. - `cargo clippy --all-targets --workspace -- -D warnings`, `make format`, and `make` are clean. -- 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]
