Skip to content

Derive UDF equality from PartialEq, Hash #16842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 25, 2025
Merged

Derive UDF equality from PartialEq, Hash #16842

merged 4 commits into from
Jul 25, 2025

Conversation

findepi
Copy link
Member

@findepi findepi commented Jul 21, 2025 •
edited
Loading

Reduce boilerplate in cases where implementation of {ScalarUDFImpl,AggregateUDFImpl,WindowUDFImpl}::{equals,hash_value} can be derived using standard PartialEq and Hash traits.

This is code complexity reduction. Follows #16781

While valuable on its own, this also prepares for more automatic derivation of UDF equals/hash and/or removal of default implementations (which currently are error-prone) -- #16677

@findepi findepi marked this pull request as draft July 21, 2025 14:20
@github-actions github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate labels Jul 21, 2025
@findepi findepi force-pushed the findepi/derive-udf-equality-from-partialeq-hash-c0f600 branch from e6bf41d to 234c978 Compare July 21, 2025 14:22
@findepi
Copy link
Member Author

findepi commented Jul 21, 2025

This is POC only for now.
@alamb @kosiew @timsaucer PTAL and let me know if you agree with the direction.

Note the PR goals:

  • reduce complexity of existing code, by deriving ScalarUDFImpl::equals and ScalarUDFImpl::hash_code from PartialEq and Hash implementations (same for AggregateUDFImpl and WindowUDFImpl)

  • if we decide to remove default implementations of ScalarUDFImpl::equals and ScalarUDFImpl::hash_code ([EPIC] ScalarUDFImpl::equals default implementation is error-prone #16677), something like this would be even more valuable. If we decide to keep them, it's still a good change.

@findepi findepi mentioned this pull request Jul 21, 2025
Closed
alamb
alamb reviewed Jul 21, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

@findepi
Derive UDF equality from PartialEq, Hash
493ac60 
Reduce boilerplate in cases where implementation of
`{ScalarUDFImpl,AggregateUDFImpl,WindowUDFImpl}::{equals,hash_code}` can
be derived using standard `PartialEq` and `Hash` traits.

This is code complexity reduction.

While valuable on its own, this also prepares for more automatic
derivation of UDF equals/hash and/or removal of default implementations
(which currently are error-prone).
@findepi findepi force-pushed the findepi/derive-udf-equality-from-partialeq-hash-c0f600 branch from 234c978 to 493ac60 Compare July 23, 2025 08:30
@findepi findepi marked this pull request as ready for review July 23, 2025 08:30
@github-actions github-actions bot added sql SQL Planner proto Related to proto crate ffi Changes to the ffi crate labels Jul 23, 2025
@findepi
Copy link
Member Author

findepi commented Jul 23, 2025 •
edited
Loading

Update. This PR covers ScalarUDFImpl that today have explicit equals/hash implementations (= closes #16865)

Next steps are defined as sub-issues in #16677

@findepi findepi requested a review from kosiew July 23, 2025 15:45
alamb
alamb approved these changes Jul 24, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @findepi -- this is a nice improvement I think

kosiew
kosiew reviewed Jul 25, 2025
View reviewed changes
Copy link
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@findepi findepi merged commit 070517a into apache:main Jul 25, 2025
27 checks passed
@findepi findepi deleted the findepi/derive-udf-equality-from-partialeq-hash-c0f600 branch July 25, 2025 19:21
@findepi
Copy link
Member Author

findepi commented Jul 25, 2025

Thank you @alamb @kosiew for review!

@xudong963 xudong963 mentioned this pull request Jul 21, 2025
Open
37 tasks
adriangb pushed a commit to pydantic/datafusion that referenced this pull request Jul 28, 2025
@findepi @adriangb
Derive UDF equality from PartialEq, Hash (apache#16842)
fdd564c 
* Derive UDF equality from PartialEq, Hash

Reduce boilerplate in cases where implementation of
`{ScalarUDFImpl,AggregateUDFImpl,WindowUDFImpl}::{equals,hash_code}` can
be derived using standard `PartialEq` and `Hash` traits.

This is code complexity reduction.

While valuable on its own, this also prepares for more automatic
derivation of UDF equals/hash and/or removal of default implementations
(which currently are error-prone).

* udf_equals_hash example

* test udf_equals_hash

* empty: roll the dice 🎲
@findepi findepi mentioned this pull request Aug 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kosiew kosiew kosiew left review comments

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate ffi Changes to the ffi crate logical-expr Logical plan and expressions proto Related to proto crate sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Derive UDF (ScalarUDFImpl) equality from PartialEq, Hash
3 participants
@findepi @alamb @kosiew