Skip to content

Derive WindowUDFImpl equality, hash from Eq, Hash traits - #17081

Merged
findepi merged 7 commits into
apache:mainfrom
findepi:findepi/udwf-perfect-eq
Aug 11, 2025
Merged

Derive WindowUDFImpl equality, hash from Eq, Hash traits#17081
findepi merged 7 commits into
apache:mainfrom
findepi:findepi/udwf-perfect-eq

Conversation

@findepi

@findepifindepi commented Aug 8, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Previously, the WindowUDFImpl trait contained equals and
hash_value methods with contracts following the Eq and Hash
traits. However, the existence of default implementations of these
methods made it error-prone, with many functions (scalar, aggregate,
window) missing to customize the equals even though they ought to.
There is no fix to this that's not an API breaking change, so a breaking
change is warranted.

Removing the default implementations would be enough of a solution, but
at the cost of a lot of boilerplate needed in implementations.

Instead, this removes the methods from the trait, and reuses DynEq,
DynHash traits used previously only for physical expressions. This
allows for functions to provide their implementations using no more than
#[derive(PartialEq, Eq, Hash)] in a typical case.

@findepifindepi added the api change Changes the API exposed to users of the crate label Aug 8, 2025
@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate proto Related to proto crate functions Changes to functions implementation ffi Changes to the ffi crate labels Aug 8, 2025
@findepifindepi mentioned this pull request Aug 8, 2025
Closed
@findepi
findepiforce-pushed the findepi/udwf-perfect-eq branch 3 times, most recently from 4e26aaa to b6fcc88CompareAugust 8, 2025 09:51
@findepifindepi changed the title Derive WindowUDFImpl eq,hash from Eq,Hash traitsDerive WindowUDFImpl equality, hash from Eq, Hash traitsAug 8, 2025
@findepi
findepiforce-pushed the findepi/udwf-perfect-eq branch 2 times, most recently from c50b82d to 58c1e4fCompareAugust 8, 2025 10:17
alamb
alamb reviewed Aug 8, 2025
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @findepi -- I think the code is looking good here

The only thing I think is missing is some additional documentation in the Upgrading.md guide to explain to people upgrading what they will need to do (add PartialEq and Eq derivation to their functions, right?)

Comment threaddatafusion/expr/src/udwf.rs
@findepi

findepi commented Aug 8, 2025

Copy link
Copy Markdown
MemberAuthor

The dependency PRs have been merged, will rebase

@findepi
Move DynEq, DynHash to expr-common
213df58
Use `DynEq` and `DynHash` traits from physical expressions crate to a
common crate for physical and logical expressions. This allows them to
be used by logical expressions.
@findepi
Derive WindowUDFImpl equality, hash from Eq, Hash traits
ab03a96
Previously, the `WindowUDFImpl` trait contained `equals` and
`hash_value` methods with contracts following the `Eq` and `Hash`
traits. However, the existence of default implementations of these
methods made it error-prone, with many functions (scalar, aggregate,
window) missing to customize the equals even though they ought to.
There is no fix to this that's not an API breaking change, so a breaking
change is warranted.
Removing the default implementations would be enough of a solution, but
at the cost of a lot of boilerplate needed in implementations.
Instead, this removes the methods from the trait, and reuses `DynEq`,
`DynHash` traits used previously only for physical expressions. This
allows for functions to provide their implementations using no more than
`#[derive(PartialEq, Eq, Hash)]` in a typical case.
@findepi
findepiforce-pushed the findepi/udwf-perfect-eq branch from 58c1e4f to ab03a96CompareAugust 8, 2025 20:23
@github-actionsgithub-actionsBot added the documentation Improvements or additions to documentation label Aug 8, 2025
@findepi
findepi requested a review from alambAugust 9, 2025 20:43
xudong963
xudong963 reviewed Aug 11, 2025
View reviewed changes
Comment threaddocs/source/library-user-guide/upgrading.md
timsaucer
timsaucer approved these changes Aug 11, 2025
View reviewed changes

@timsaucertimsaucer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

kosiew
kosiew reviewed Aug 11, 2025
View reviewed changes

@kosiewkosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments for your consideration.

Comment threaddatafusion/expr/src/udwf.rs Outdated
Comment threaddatafusion/expr/src/udwf.rs Outdated
Comment threaddatafusion/expr-common/src/dyn_eq.rs Outdated
alamb
alamb approved these changes Aug 11, 2025
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @findepi@xudong963 and @kosiew

Comment threaddatafusion/expr-common/src/dyn_eq.rs Outdated
Comment threaddocs/source/library-user-guide/upgrading.md Outdated
findepiand others added 2 commits August 11, 2025 19:24
@findepi
Seal DynEq, DynHash
acc9899
@findepi@alamb
Link to epic in upgrade guide
2859e50
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@findepi

findepi commented Aug 11, 2025

Copy link
Copy Markdown
MemberAuthor

All comments addressed. Thank you @alamb@xudong963@timsaucer@kosiew for your diligent reviews!

@findepi
findepi merged commit 8494a39 into apache:mainAug 11, 2025
28 checks passed
@findepi
findepi deleted the findepi/udwf-perfect-eq branch August 11, 2025 19:31
@findepifindepi mentioned this pull request Aug 11, 2025
Merged
@findepi

findepi commented Aug 11, 2025

Copy link
Copy Markdown
MemberAuthor

Aggregates

@findepifindepi mentioned this pull request Aug 13, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kosiewkosiewkosiew left review comments
@alambalambalamb approved these changes
@timsaucertimsaucertimsaucer approved these changes
@xudong963xudong963xudong963 approved these changes

Assignees

No one assigned

Labels

api changeChanges the API exposed to users of the cratecoreCore DataFusion cratedocumentationImprovements or additions to documentationffiChanges to the ffi cratefunctionsChanges to functions implementationlogical-exprLogical plan and expressionsoptimizerOptimizer rulesphysical-exprChanges to the physical-expr cratesprotoRelated to proto crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace WindowUDFImpl::{equals,hash_value} with UdfHash, UdfEq traits Implement PartialEq, Hash for all UDWFs (WindowUDFImpl)

5 participants

@findepi@alamb@timsaucer@kosiew@xudong963