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

kosiew · 2025-07-04T10:09:13Z

Which issue does this PR close?

Closes #16677

Rationale for this change

The previous behavior of ScalarUDFImpl::equals relied on comparing only type IDs, names, and signatures, which could lead to incorrect assumptions about equality—especially for UDFs with internal state.

This change introduces a pointer-based default equality implementation, ensuring distinct instances are not erroneously considered equal. It also encourages users to override equals for more precise behavior when necessary.

What changes are included in this PR?

Updated the default implementation of ScalarUDFImpl::equals to check for pointer equality only.
Added logic in ScalarUDF's PartialEq to check for Arc::ptr_eq before delegating to equals.
Introduced a new test suite in expr/src/test/udf_equals.rs covering various equality scenarios.
Added a unit test test_scalar_udf_pointer_equality in projection_pushdown.rs.
Documented the change and its implications in upgrading.md, including migration guidance and example implementation.

Are these changes tested?

Yes ✅
Extensive unit tests are provided to validate:

Different and same-instance comparisons.
Type-based mismatches.
Custom UDFs implementing their own equality logic.

Are there any user-facing changes?

Yes ✅

Custom UDF authors must now override equals and hash_value if they expect equivalence behavior beyond pointer equality.
The change is backward-compatible for users who do not rely on ScalarUDF equality.
Detailed upgrade notes are provided in the documentation.

…son logic

… comparison logic

…mentation

…logic

…quality rules and provide implementation examples

…ementation

…lity

findepi · 2025-07-04T11:52:38Z

datafusion/expr/src/test/udf_equals.rs

+    }
+    fn equals(&self, other: &dyn ScalarUDFImpl) -> bool {
+        if let Some(other) = other.as_any().downcast_ref::<ParamUdf>() {
+            self.param == other.param && self.type_id() == other.type_id()


Is it intentional not to compare signature? if ues, let's add a code comment
if no, the ParamUdf could use derive(PartialEq) and then self == other here

Good point.
Amended to implement
derive(PartialEq) and then self == other

findepi · 2025-07-04T11:53:03Z

datafusion/expr/src/test/udf_equals.rs

+    hash::{Hash, Hasher},
+};
+#[derive(Debug)]
+#[allow(dead_code)]


can this be removed?

findepi · 2025-07-04T11:54:23Z

datafusion/expr/src/udf.rs

+        // if the types, names and signatures are the same, we can't know if they are the same so we
+        // assume they are not.
+        // If a UDF has internal state that should be compared, it should implement this method
+        false


Could we do bit-by-bit comparison of the self & other, if they are the same type?

amended comment to clarify why we are not doing
bit-by-bit comparison

// Alternative approach: we could potentially do bit-by-bit comparison if both objects // are the same concrete type, but this requires: // 1. Both objects to have identical TypeId // 2. Careful handling of potential padding bytes in structs // 3. The concrete type to be safely comparable via memcmp // For now, we use the conservative approach of returning false

// 1. Both objects to have identical TypeId

we know typeid via as_any(), right?

// 2. Careful handling of potential padding bytes in structs

i don't know if rust does something (zeros) with padding bytes

// 3. The concrete type to be safely comparable via memcmp

rust struct is generally moveable around. i think the move semantics are generally about memcpy-ing bits to a new location, so memcmp-ing bits should be fine.

findepi · 2025-07-04T11:56:53Z

docs/source/library-user-guide/upgrading.md

@@ -62,6 +62,36 @@ DataFusionError::SchemaError(

 [#16652]: https://github.com/apache/datafusion/issues/16652

+#### `ScalarUDFImpl::equals` Default Implementation
+
+The default implementation of the `equals` method in the `ScalarUDFImpl` trait has been updated. Previously, it compared only the type IDs, names, and signatures of UDFs. Now, it assumes UDFs are not equal unless their pointers are the same.


This is safe default. Executing this upgrade suggestion is necessary for common expression elimination to work.
Is it worth considering doing the change more as a breaking change and require explicit upgrade step?

It's more work for sure, but also -- the result is much more predictible.

For example, we could require ScalarUDFImpl to implement PartialEq and delegate to it.

That's a thoughtful consideration. You raise valid points about predictability vs. convenience. Let me outline the tradeoffs:

Current approach (safe default with pointer comparison):

✅ Prevents silent bugs where ScalarUDFs with different internal state are incorrectly considered equal
✅ Forces developers to think about equality semantics for their specific UDFs
✅ Common expression elimination works correctly out of the box
❌ Requires manual action from users who want structural equality

Alternative approach (require explicit PartialEq implementation):

✅ More explicit and predictable - no hidden behavior changes
✅ Leverages Rust's type system for compile-time guarantees
✅ Clear contract: if you want equality comparison, implement PartialEq
❌ Breaking change requiring immediate action from all ScalarUDF implementors
❌ More work for simple ScalarUDFs that don't need custom equality logic

My recommendation: Stick with the current approach for DataFusion 49.0.0, but consider the PartialEq approach for a future major version. Here's why:

The current change already enables critical functionality (common expression elimination) without breaking compilation
The upgrade path is clear and documented with examples
For DataFusion 50.0.0 or 51.0.0, we could introduce a more explicit API that requires PartialEq implementation
This gives users time to adapt while ensuring the optimizer works correctly immediately.

What do you think about this phased approach?

findepi · 2025-07-04T11:57:50Z

datafusion/expr/src/udf.rs

+    ///    }
+    ///     fn equals(&self, other: &dyn ScalarUDFImpl) -> bool {
+    ///         if let Some(other) = other.as_any().downcast_ref::<Self>() {
+    ///             self.param == other.param && self.name() == other.name()


Why not include signature in the equality check?
Worth code comment

Amended to use
derive PartialEq

datafusion/expr/src/udf.rs

datafusion/expr/src/test/udf_equals.rs

findepi · 2025-07-04T11:59:25Z

datafusion/expr/src/test/mod.rs

@@ -16,3 +16,5 @@
 // under the License.

 pub mod function_stub;
+#[cfg(test)]
+pub mod udf_equals;


shouldn't udf_equals.rs simply go into tests/ folder (datafusion/expr/tests)?

Good idea.
Moved.

…ons for improved clarity and correctness

…comparison approach details

Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>

…ck in equals method

findepi · 2025-07-15T08:41:46Z

datafusion/expr/src/udf.rs

+        // if the types, names and signatures are the same, we can't know if they are the same so we
+        // assume they are not.
+        // If a UDF has internal state that should be compared, it should implement this method
+        false


// 1. Both objects to have identical TypeId

we know typeid via as_any(), right?

// 2. Careful handling of potential padding bytes in structs

i don't know if rust does something (zeros) with padding bytes

// 3. The concrete type to be safely comparable via memcmp

rust struct is generally moveable around. i think the move semantics are generally about memcpy-ing bits to a new location, so memcmp-ing bits should be fine.

findepi · 2025-07-15T08:43:17Z

datafusion/expr/src/udf.rs

    fn equals(&self, other: &dyn ScalarUDFImpl) -> bool {
-        self.name() == other.name() && self.signature() == other.signature()
+        // if the pointers are the same, the UDFs are the same
+        if std::ptr::eq(self.as_any(), other.as_any()) {


Could this be std::ptr::eq(self, other) here?

I think we cannot safely use std::ptr::eq(self, other) here because self and other are trait objects (&dyn ScalarUDFImpl).
std::ptr::eq(self, other) would compare the vtable pointers, not the underlying data pointers, and may return false even if the trait objects point to the same data.

findepi · 2025-07-15T08:49:03Z

"99%" of the cases are stateless¹. Would it be possible to provide default implementation that works great in the "99%" common case, but still works correctly in other cases?

if that's not possible, and so the default behavior is inferior for the common case, it needs to be reimplemented in every function. Which means it's not really an optional method and so we should remove its default implementation.

cc @alamb

the only field in most UDFs is Signature. It's there for convenience. Could be as well implemented as a static ↩

…ety and handle padding considerations

alamb · 2025-07-17T21:50:39Z

I think this PR is an alternative to the proposed fix in

Implement equals for stateful functions #16781

Is that right? Are you happy with the code in #16781 @kosiew ?

findepi · 2025-07-18T09:24:01Z

@alamb #16781 aims to solve immediate obvious problem - incorrect implementation of equality for stateful functions managed in this repo. That PR does not solve #16677.

This PR aims to solve the problem of equality being error-prone, wrong by default. I.e. addresses #16677.
The #16781 PR shows how wide-spread the problem is, giving more motivation to fix the default behavior.

My proposal is

land Implement equals for stateful functions #16781 (or equivalent), i.e. fix the immediate problem
remove default implementation of equality -- this is a breaking change

alamb · 2025-07-18T13:02:10Z

remove default implementation of equality -- this is a breaking change

As long as we sufficiently document how to fix it (with an example in the upgrade guide) I I think that makes sense to me

Probably in DataFusion 50

findepi · 2025-07-19T13:12:32Z

Posted a solution proposal -- #16677 (comment)

kosiew · 2025-07-25T04:01:01Z

Closing this in favour of #16677 (comment)

kosiew added 10 commits July 4, 2025 18:05

Add UDF equality checks and new test module for UDFs

288313e

Enhance equality check for Scalar UDFs to improve accuracy in compari…

37a37f1

…son logic

Add equality tests for Scalar UDF instances and types

b49303e

Refactor equality check in ScalarUDFImpl to simplify comparison logic

0184ef2

Implement equality checks for ParamUdf and AnotherParamUdf to enhance…

3b1bd00

… comparison logic

Refactor AnotherParamUdf to SignatureUdf for consistency in UDF imple…

d0b1318

…mentation

Add DefaultParamUdf implementation and equality tests for comparison …

6128779

…logic

Enhance documentation for equals method in ScalarUDFImpl to clarify e…

579f3c4

…quality rules and provide implementation examples

Add hash_value method to ParamUdf for improved equality checks

83b7d35

Document breaking change in ScalarUDFImpl::equals method default impl…

a7a8c02

…ementation

github-actions bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions labels Jul 4, 2025

kosiew added 3 commits July 4, 2025 18:11

Add missing import for ScalarUDFImpl in udf.rs

c303071

Add hash_value method to ScalarUDFImpl for improved hashing functiona…

186f70d

…lity

Merge branch 'main' into udf-16677

71ba61e

kosiew force-pushed the udf-16677 branch from c811b17 to 71ba61e Compare July 4, 2025 10:11

kosiew added 2 commits July 4, 2025 18:40

Add license header to udf_equals.rs

88afc7a

fix prettier errors

7e41e4e

findepi reviewed Jul 4, 2025

View reviewed changes

Improve ScalarUDF equality check for pointer equality optimization

ec5dcb3

github-actions bot added the core Core DataFusion crate label Jul 4, 2025

kosiew changed the title ~~Improve ScalarUDFImpl::equals Default Behavior and Add Unit Tests for Custom Equality~~ Enhance ScalarUDFImpl Equality Handling with Pointer-Based Default and Customizable Logic Jul 4, 2025

kosiew and others added 8 commits July 7, 2025 09:39

Refactor: derive PartialEq ParamUdf equality and hashing implementati…

8dc7e11

…ons for improved clarity and correctness

Enhance ScalarUDFImpl equality method documentation with alternative …

676545c

…comparison approach details

Minor comment adjustment of use statement

dc6264b

Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>

Add blankline

d5b813b

Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>

Merge branch 'udf-16677' of github.com:kosiew/datafusion into udf-16677

06d15b8

Refactor: derive PartialEq for MyUdf struct and simplify equality che…

2ee9c8f

…ck in equals method

Merge branch 'main' into udf-16677

571f333

Move udf_equals module to datafusion/expr/tests

f9c8408

kosiew added 6 commits July 7, 2025 17:06

Fix doc tests

73678db

Fix prettier error

ec7930c

Merge branch 'main' into udf-16677

6be196d

resolve merge conflict

dd4aec2

fix prettier error

67db2df

Merge branch 'main' into udf-16677

db39478

kosiew force-pushed the udf-16677 branch from 596dbcd to db39478 Compare July 11, 2025 05:29

kosiew added 2 commits July 12, 2025 18:58

Merge branch 'main' into udf-16677

34a612e

Merge branch 'main' into udf-16677

0b92276

kosiew requested a review from findepi July 12, 2025 12:12

kosiew added 2 commits July 12, 2025 21:46

Merge branch 'main' into udf-16677

960e845

Merge branch 'main' into udf-16677

9a16939

findepi reviewed Jul 15, 2025

View reviewed changes

kosiew added 2 commits July 15, 2025 17:26

refactor: improve equality check for ScalarUDFImpl to ensure type saf…

e8f22aa

…ety and handle padding considerations

refactor: streamline use

1581b76

findepi mentioned this pull request Jul 15, 2025

Implement equals for stateful functions #16781

Merged

kosiew added 2 commits July 15, 2025 19:10

Merge branch 'main' into udf-16677

9993abb

fix clippy error

3e466d7

kosiew closed this Jul 25, 2025

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

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

Uh oh!

Conversation

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

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Jul 15, 2025

Footnotes

Uh oh!

alamb commented Jul 17, 2025

Uh oh!

findepi commented Jul 18, 2025

Uh oh!

alamb commented Jul 18, 2025

Uh oh!

findepi commented Jul 19, 2025

Uh oh!

kosiew commented Jul 25, 2025

Uh oh!

Uh oh!

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

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

kosiew commented Jul 4, 2025 •

edited

Loading