Implement equals for stateful functions by findepi · Pull Request #16781 · apache/datafusion

findepi · 2025-07-15T09:42:43Z

Default implementation of ScalarUDFImpl::equals, AggregateUDFImpl::equals and WindowUDFImpl::equals is correct for stateless functions and those which only state is the Signature, which is most of the functions.

This implements equals and hash_value for functions that have state other than Signature object.

This fixes correctness issues which could occur when such stateful functions are used together in one query.

relates to [EPIC] ScalarUDFImpl::equals default implementation is error-prone #16677
relates to Enhance ScalarUDFImpl Equality Handling with Pointer-Based Default and Customizable Logic #16681

Default implementation of `ScalarUDFImpl::equals`, `AggregateUDFImpl::equals` and `WindowUDFImpl::equals` is correct for stateless functions and those which only state is the `Signature`, which is most of the functions. This implements `equals` and `hash_value` for functions that have state other than `Signature` object. This fixes correctness issues which could occur when such stateful functions are used together in one query.

kosiew

Left a suggestion and a question.

datafusion/core/tests/user_defined/user_defined_aggregates.rs

datafusion-examples/examples/function_factory.rs

timsaucer

The approach looks very reasonable, but there is a lot of boiler plate code. I'm guessing there wasn't an easier way to add some derived traits for PartialEq and Hash for these structs instead. I'll try to set aside some time to go through each of these, but it's going to take a bit.

findepi · 2025-07-15T12:03:14Z

The approach looks very reasonable, but there is a lot of boiler plate code. I'm guessing there wasn't an easier way to add some derived traits for PartialEq and Hash for these structs instead.

It would work for some, but not all.
If we want ptr eq for an Arc, we need hand-written eq and hash anyway.

alamb

Thank you @findepi and @kosiew and @timsaucer -- I think this PR makes sense to me and I like how explicit it is.

It took a while to get through 😅

I think it could be reduced in size somewhat by adding aliases to the default implementations.

My biggest concern is that in the future when we add new functions / modify existing ones we will forget to add a corresponding equals and hash implementations

While I also don't like the amount of almost-identical copy/paste code in this PR I don't really have any better suggestion and thus I think this PR is an improvement over main

To try and mitigage the issues, I suggest:

Add a comment to all the implementations added in this PR like // Implement equals because there are fields other than name and signature and // implement hash because there are fields other than name and signature

Maybe update the comments in

datafusion/datafusion/expr/src/udf.rs

Lines 697 to 706 in 4c877f6

    
               /// Return true if this scalar UDF is equal to the other. 
        
               /// 
        
               /// Allows customizing the equality of scalar UDFs. 
        
               /// Must be consistent with [`Self::hash_value`] and follow the same rules as [`Eq`]: 
        
               /// 
        
               /// - reflexive: `a.equals(a)`; 
        
               /// - symmetric: `a.equals(b)` implies `b.equals(a)`; 
        
               /// - transitive: `a.equals(b)` and `b.equals(c)` implies `a.equals(c)`. 
        
               /// 
        
               /// By default, compares [`Self::name`] and [`Self::signature`].

and

datafusion/datafusion/expr/src/udf.rs

Lines 711 to 716 in 4c877f6

    
               /// Returns a hash value for this scalar UDF. 
        
               /// 
        
               /// Allows customizing the hash code of scalar UDFs. Similarly to [`Hash`] and [`Eq`], 
        
               /// if [`Self::equals`] returns true for two UDFs, their `hash_value`s must be the same. 
        
               /// 
        
               /// By default, hashes [`Self::name`] and [`Self::signature`].

to say when the functions should be overridden (for example "If your implementation has fields other than signature and name, such as aliases, you likely should implement equals and hash as well`)

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

datafusion/doc/src/lib.rs

datafusion/functions-nested/src/sort.rs

findepi · 2025-07-16T07:39:01Z

I think it could be reduced in size somewhat by adding aliases to the default implementations.

i'd rather see stuff like aliases and documentation not be part of ScalardUDFImpl at all.
The logical plan doesn't have to bear documentation of its constituents.

But yeah, i can add aliases there.

My biggest concern is that in the future when we add new functions / modify existing ones we will forget to add a corresponding equals and hash implementations

This is absolutely NOT improved by this PR. The problem remains unsolved.

To try and mitigage the issues, I suggest:

Add a comment to all the implementations added in this PR like // Implement equals because there are fields other than name and signature and // implement hash because there are fields other than name and signature

I don't think it helps.
Whenever you see equals/hash functions you know what they are for.
The crux of the problem is -- you need to be aware of their existence in the ScalardUDFImpl to even implement them!
I'd suggest removing default implementations if we cannot make them correct. Not in this PR.

remove these which compare signature, aliases, as the default handles these now

findepi · 2025-07-17T13:49:48Z

@alamb @timsaucer @kosiew would you like to take a look at new code pushed here since the time you last reviewed?

alamb

I looked at the commits since I last reviewed and they look good to me -- thank you @findepi

* Implement equals for stateful functions Default implementation of `ScalarUDFImpl::equals`, `AggregateUDFImpl::equals` and `WindowUDFImpl::equals` is correct for stateless functions and those which only state is the `Signature`, which is most of the functions. This implements `equals` and `hash_value` for functions that have state other than `Signature` object. This fixes correctness issues which could occur when such stateful functions are used together in one query. * downgrade for MSRV * Improve doc * Update default UDF:: equals to compare aliases too * Update default UDF:: equals to compare type too (‼️) * remove now-obsoleted UDF equals/hash customizations remove these which compare signature, aliases, as the default handles these now * remove equals impl which compares name, signature -- default does that * cleanup imports (cherry picked from commit afd8235)

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate proto Related to proto crate functions Changes to functions implementation ffi Changes to the ffi crate spark labels Jul 15, 2025

findepi force-pushed the findepi/equals branch from 63bbe1d to 4c877f6 Compare July 15, 2025 09:49

findepi requested review from alamb, kosiew and timsaucer July 15, 2025 09:50

kosiew reviewed Jul 15, 2025

View reviewed changes

datafusion/core/tests/user_defined/user_defined_aggregates.rs Show resolved Hide resolved

datafusion-examples/examples/function_factory.rs Show resolved Hide resolved

downgrade for MSRV

a0db32d

timsaucer reviewed Jul 15, 2025

View reviewed changes

findepi mentioned this pull request Jul 15, 2025

Backport: Implement equals for stateful functions sdf-labs/datafusion#98

Merged

alamb approved these changes Jul 15, 2025

View reviewed changes

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs Show resolved Hide resolved

datafusion/doc/src/lib.rs Show resolved Hide resolved

datafusion/functions-nested/src/sort.rs Outdated Show resolved Hide resolved

findepi added 5 commits July 16, 2025 09:47

Improve doc

bc55bed

Update default UDF:: equals to compare aliases too

492c446

Update default UDF:: equals to compare type too (‼️)

6016142

remove now-obsoleted UDF equals/hash customizations

cb18579

remove these which compare signature, aliases, as the default handles these now

remove equals impl which compares name, signature -- default does that

f27480a

github-actions bot removed the spark label Jul 16, 2025

cleanup imports

4c954ec

findepi force-pushed the findepi/equals branch from 2019005 to 4c954ec Compare July 16, 2025 15:16

alamb approved these changes Jul 17, 2025

View reviewed changes

alamb mentioned this pull request Jul 17, 2025

Enhance ScalarUDFImpl Equality Handling with Pointer-Based Default and Customizable Logic #16681

Closed

findepi mentioned this pull request Jul 18, 2025

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

Closed

findepi merged commit afd8235 into apache:main Jul 18, 2025
27 checks passed

findepi deleted the findepi/equals branch July 18, 2025 09:27

findepi mentioned this pull request Jul 21, 2025

Derive UDF equality from PartialEq, Hash #16842

Merged

Flyangz mentioned this pull request Jan 16, 2026

Fix ext function caching apache/auron#1902

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement equals for stateful functions#16781

Implement equals for stateful functions#16781
findepi merged 8 commits intoapache:mainfrom
findepi:findepi/equals

findepi commented Jul 15, 2025 •

edited

Loading

Uh oh!

kosiew left a comment

Uh oh!

Uh oh!

Uh oh!

timsaucer left a comment

Uh oh!

findepi commented Jul 15, 2025

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi commented Jul 16, 2025

Uh oh!

findepi commented Jul 17, 2025

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/// Return true if this scalar UDF is equal to the other.
	///
	/// Allows customizing the equality of scalar UDFs.
	/// Must be consistent with [`Self::hash_value`] and follow the same rules as [`Eq`]:
	///
	/// - reflexive: `a.equals(a)`;
	/// - symmetric: `a.equals(b)` implies `b.equals(a)`;
	/// - transitive: `a.equals(b)` and `b.equals(c)` implies `a.equals(c)`.
	///
	/// By default, compares [`Self::name`] and [`Self::signature`].

	/// Returns a hash value for this scalar UDF.
	///
	/// Allows customizing the hash code of scalar UDFs. Similarly to [`Hash`] and [`Eq`],
	/// if [`Self::equals`] returns true for two UDFs, their `hash_value`s must be the same.
	///
	/// By default, hashes [`Self::name`] and [`Self::signature`].

Conversation

findepi commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

timsaucer left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Jul 15, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi commented Jul 16, 2025

Uh oh!

findepi commented Jul 17, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

findepi commented Jul 15, 2025 •

edited

Loading