ScalarUDFImpl::equals default implementation is error-prone

[findepi]

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.

[alamb]

🤔 That is a tricky business

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

[findepi]

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.

[findepi]

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.

[findepi]

#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.

[alamb]

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