apache / apache/datafusion-comet
Honor the deterministic flag for Rust UDFs
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
Follow-up from review of #4459.
`CometRustUDF.register` accepts a `deterministic` parameter and carries it in the `RustUdfCall` proto, but the native side ignores it: `ImportedCScalarUdf::try_new` hardcodes `Volatility::Immutable`. A nondeterministic UDF planned as immutable may be constant-folded, evaluated once and reused, or eliminated as a common subexpression.
#4459 closes the hole by rejecting `deterministic = false` at registration, so the flag cannot silently lie. This issue is about actually supporting it.
The obstacle is lifetime mismatch. The `Signature` is built once per library load and cached process-wide per `(library_path)`, while determinism is declared per `CometRustUDF.register` call. Two registrations of the same kernel with different determinism would need different volatility from one cached `ScalarUDFImpl`.
Options worth weighing:
- Key the cached adapter on `(library_path, name, volatility)` rather than on the library alone.
- Build the `ScalarUDFImpl` per call site in the planner from the cached kernel, taking volatility from `RustUdfCall.deterministic`, and keep only the loaded library in the cache.
- Let the kernel declare its own volatility over the ABI and validate the registration against it, which overlaps with the `get_property` idea in #5254.
Once this lands, drop the guard in `CometRustUDF.register`, the note on the `deterministic` field in `expr.proto`, the "Immutable functions only" limitation in `docs/source/user-guide/latest/rust_udfs.md`, and the corresponding note on the `CometCScalarUdf` trait docs.
Contributor guide
Research direction
Start with CometRustUDF.register, RustUdfCall, and ImportedCScalarUdf::try_new to trace how determinism reaches native planning and how the library cache is keyed. Compare the options with #4459 and the get_property discussion in #5254. Done means nondeterministic registrations retain the correct volatility, the registration guard and documented immutable-only limitations are removed from expr.proto, rust_udfs.md, and the CometCScalarUdf trait docs.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust, scala
- Domain
- backend, documentation
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100