[EPIC] ScalarUDFImpl::equals default implementation is error-prone

When you look at ScalarUDFImpl::equals, there is nothing wrong with it.
Problems happen when you do not look at it:

case a)

add a normal struct implementing a function, i.e. implement ScalarUDFImpl trait
add tests, perhaps with slt
... some time later ...
make the function parameterized (add new field, like "time zone", "default value" or similar)
add tests, perhaps with slt

case b)

in the same LogicalPlan put two functions implemented by two different structs, for example regex_replace(string, string, string) with different regex engines

In both cases everything works great in tests, yet the real queries may fail, because the default implementation of ScalarUDFImpl::equals silently incorrect.
In case (a) it was because of evolution of the code.
In case (b) it was merely because two different functions can have same name & signature.

Expected

Default implementation of a public trait method should be less easy to get wrong.

🤔 That is a tricky business

Maybe ScalarUDFImpl needs some way to have eq called on it too 🤔

Worth noting

Implement equals for stateful functions #16781

That PR adds missing equality implementation to many functions and improves default implementation to consider type_id and aliases. It well shows how "wrong by default" equality turned out to be a real problem in practice even within this code base. We don't know about other codebases, but for dbt it was a problem as well, leading to hard-to-understand query failures.

How we can solve this:

Add two new traits

trait UdfHash {
    fn udf_hash(&self) -> u64;
}

trait UdfEq {
    fn udf_eq(&self, other: &dyn Any) -> bool;
}

with blanket implementations

impl<T: Hash + Any> UdfHash for T {
    fn udf_hash(&self) -> u64 {
        let hasher = &mut DefaultHasher::new();
        self.type_id().hash(hasher);
        self.hash(hasher);
        hasher.finish()
    }
}

impl<T: PartialEq + Any> UdfEq for T {
    fn udf_eq(&self, other: &dyn Any) -> bool {
        let Some(other) = other.downcast_ref::<Self>() else {
            return false;
        };
        self == other
    }
}

Make `ScalarUDFImpl` depend on these

trait ScalarUDFImpl: Send + Sync + UdfHash + UdfEq + Any {
 ...
}

Usage

#[derive(Debug, Clone, PartialEq, Hash)]
struct MyFunc {
    state: SomeEquitableStateType,
}

impl ScalarUDFImpl for MyFunc {
   // nothing eq/hash related needed here
}

As shown, the end result is that ScalarUDFImpl can be hashed and compared for equality and all the implementor needs is #[derive(PartialEq, Hash)] to make it work.

Notes

`UdfEq` accepts any `Any`

UdfEq accepts any Any so it's possible to call it with instances other than &dyn ScalarUDFImpl.
I don't know how to avoid this.
In particular, this is not allowed in Rust:

// Error: cycle detected when computing the super predicates of `ScalarUDFImpl` [E0391]
trait ScalarUDFImpl: PartialEq<dyn ScalarUDFImpl> {
 /* ... */
}

We can cover this with additional eq/hash methods directly in ScalarUDFImpl trait that will have default implementation and will not need to be reimplemented.

trait ScalarUDFImpl: Send + Sync + UdfHash + UdfEq + Any {
    /* ... */
    
    fn eq(&self, other: &dyn ScalarUDFImpl) -> bool {
        self.udf_eq(other)
    }
    
    fn hash(&self) -> u64 {
        self.udf_hash()
    }
}

Compiler errors don't make it obvious

As with any trait with blanket implementations (such as Into), when you don't implement it yet, the compiler won't tell you how you should implement.
For example, if a function lacks PartialEq implementation, the compiler will complain about UdfEq instead.

// Missing eq implementation
#[derive(Debug, Clone, /*PartialEq,*/ Hash)]
struct MyFunc {
    safe: bool,
}

// Error: the trait bound `MyFunc: UdfEq` is not satisfied
impl ScalarUDFImpl for MyFunc {
    /* ... */
}

It can be mitigated by a good documentation for the UdfEq, UdfHash traits informing about blanket implementation and that they should not be implemented directly.

#16842 is a POC how we can convert hand-written equals, hash_value implementations into PartialEq and Hash traits, which would be good preparatory step for the plan proposed above.

I think this idea sounds great -- as long as it is documented (which I think we can given your description) it will be great

I have defined sub tasks in this issue.
I want to use ScalarUDFImpl as the design driver / place where review happens. The AggregateUDFImpl and WindowUDFImpl can be worked on independently.

@alamb

The error-proneness is very well exemplified by

Support multiple ordered array_agg aggregations #16625 introducing a bug
- which is later fixed in Fix equality of parametrizable ArrayAgg function #17065

The code author or the first PR was generally well-aware of existence of equals/hash_value contract on UDFs and so were the reviewers, yet the bug was made. Subtle but dangerous.

To me it makes it clear that we really must fix the design and make it much harder to write incorrect code, even at the cost of downstream projects needing an update.

cc @alamb

This PR introduces the proposed change for window functions:

Derive WindowUDFImpl equality, hash from Eq, Hash traits #17081

Aggregates

Derive AggregateUDFImpl equality, hash from Eq, Hash traits #17130

Navigation Menu

Repository navigation

[EPIC] ScalarUDFImpl::equals default implementation is error-prone #16677

Expected

Add two new traits

Make `ScalarUDFImpl` depend on these

Usage

Notes

`UdfEq` accepts any `Any`

Compiler errors don't make it obvious

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[EPIC] ScalarUDFImpl::equals default implementation is error-prone #16677

Description

Expected

Activity

alamb commented on Jul 4, 2025

findepi commented on Jul 18, 2025

findepi commented on Jul 19, 2025

Add two new traits

Make ScalarUDFImpl depend on these

Usage

Notes

UdfEq accepts any Any

Compiler errors don't make it obvious

findepi commented on Jul 21, 2025

alamb commented on Jul 21, 2025

findepi commented on Jul 23, 2025

findepi commented on Aug 7, 2025

findepi commented on Aug 8, 2025

findepi commented on Aug 11, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Make `ScalarUDFImpl` depend on these

`UdfEq` accepts any `Any`