Repository navigation
[EPIC] ScalarUDFImpl::equals default implementation is error-prone #16677
Description
Activity
🤔 That is a tricky business
Maybe ScalarUDFImpl needs some way to have
eqcalled on it too 🤔Worth noting
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.
Reacted by Andrew LambHow 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
ScalarUDFImpldepend on thesetrait 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
ScalarUDFImplcan be hashed and compared for equality and all the implementor needs is#[derive(PartialEq, Hash)]to make it work.Notes
UdfEqaccepts anyAnyUdfEqaccepts anyAnyso 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
ScalarUDFImpltrait 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 lacksPartialEqimplementation, the compiler will complain aboutUdfEqinstead.// 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,UdfHashtraits informing about blanket implementation and that they should not be implemented directly.Reacted by Bruce RitchieI think this idea sounds great -- as long as it is documented (which I think we can given your description) it will be great
- added sub-issues
on Jul 23, 2025 I have defined sub tasks in this issue.
I want to useScalarUDFImplas the design driver / place where review happens. TheAggregateUDFImplandWindowUDFImplcan be worked on independently.The error-proneness is very well exemplified by
- Support multiple ordered
array_aggaggregations #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
Reacted by kosiew and Andrew Lamb- Support multiple ordered
This PR introduces the proposed change for window functions:
- changed the title
[-]ScalarUDFImpl::equals default implementation is error-prone[/-][+][EPIC] ScalarUDFImpl::equals default implementation is error-prone[/+]on Aug 8, 2025
When you look at
ScalarUDFImpl::equals, there is nothing wrong with it.Problems happen when you do not look at it:
case a)
ScalarUDFImpltraitcase b)
regex_replace(string, string, string)with different regex enginesIn both cases everything works great in tests, yet the real queries may fail, because the default implementation of
ScalarUDFImpl::equalssilently 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.