apache / apache/datafusion-comet

Honor the deterministic flag for Rust UDFs

Open
#5,249 0 comments 0 reactions 0 assignees View on GitHub
enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.