Skip to content

Fix hash/equality issues for ScalarFunctionExpr - #17078

Merged
findepi merged 5 commits into
apache:mainfrom
findepi:findepi/derive-dyn-eq-hash-for-scalarfunctionexpr-from-plain-eq-hash-c510be
Aug 8, 2025
Merged

Fix hash/equality issues for ScalarFunctionExpr #17078
findepi merged 5 commits into
apache:mainfrom
findepi:findepi/derive-dyn-eq-hash-for-scalarfunctionexpr-from-plain-eq-hash-c510be

Conversation

@findepi

@findepifindepi commented Aug 8, 2025

Copy link
Copy Markdown
Member
  • Fix DynEq implementation for ScalarFunctionExpr; it could return true for non equal values
  • Fox DynHash implementation for ScalarFunctionExpr to be consistent with equals

@findepi
Fix equality impl for ScalarFunctionExpr
b8e049d
The equality implementation for ScalarFunctionExpr could incorrectly
return true when one compared object has more config options. This is
because the `iterator.zip(other)` terminates whenever one of the zipped
iterators terminates.
@findepi
Fix hash/equality inconsistency for ScalarFunctionExpr
2a94bee
The `ScalarFunctionExpr` equality compares config options ignoring the
order, and so hash should ignore order too.
@findepi
Derive dyn eq,hash for ScalarFunctionExpr from plain eq, hash
e0aa151
Use standard pattern for implementing `DynEq` and `DynHash` -- they
should be derived from `Eq` and `Hash` implementations.
@github-actionsgithub-actionsBot added the physical-expr Changes to the physical-expr crates label Aug 8, 2025
@findepi
findepi requested review from alamb and compheadAugust 8, 2025 07:27
@findepifindepi mentioned this pull request Aug 8, 2025
Merged
@alambalamb mentioned this pull request Aug 8, 2025
Merged
alamb
alamb approved these changes 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.

Thank you @findepi

Comment threaddatafusion/physical-expr/src/scalar_function.rs Outdated
comphead
comphead approved these changes Aug 8, 2025
View reviewed changes

@compheadcomphead 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 just to refresh my memory https://doc.rust-lang.org/std/cmp/trait.Eq.html so
a == b will effectively call PartialEq and we dont need separate impl for Eq?

@findepi

findepi commented Aug 8, 2025

Copy link
Copy Markdown
MemberAuthor

a == b will effectively call PartialEq

yes

and we dont need separate impl for Eq?

Eq is a marker trait. Implementing Eq is a promise that PartialEq is a well-behaved equality, including reflexivity (a == a for every a). The DynEq trait used in PhysicalExpr definition currently lacks any verbal contract, but it can be reasonably expected that it behaves as Eq, including reflexivity.
This conclusion is based on the blanked implementation: impl<T: Eq + Any> DynEq for T.

@findepi
findepi merged commit 5f95734 into apache:mainAug 8, 2025
27 checks passed
@findepi
findepi deleted the findepi/derive-dyn-eq-hash-for-scalarfunctionexpr-from-plain-eq-hash-c510be branch August 8, 2025 19:08
@findepifindepi mentioned this pull request Aug 8, 2025
Merged
shruti2522 pushed a commit to shruti2522/datafusion that referenced this pull request Aug 10, 2025
@findepi@shruti2522
Fix hash/equality issues for ScalarFunctionExpr (apache#17078)
adfa251
* Fix equality impl for ScalarFunctionExpr
The equality implementation for ScalarFunctionExpr could incorrectly
return true when one compared object has more config options. This is
because the `iterator.zip(other)` terminates whenever one of the zipped
iterators terminates.
* Fix hash/equality inconsistency for ScalarFunctionExpr
The `ScalarFunctionExpr` equality compares config options ignoring the
order, and so hash should ignore order too.
* Derive dyn eq,hash for ScalarFunctionExpr from plain eq, hash
Use standard pattern for implementing `DynEq` and `DynHash` -- they
should be derived from `Eq` and `Hash` implementations.
* Fix doc example for implementing PhysicalExpr
* Short-circuit expensive operations in eq impl for ScalarFunctionExpr
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@compheadcompheadcomphead approved these changes
+1 more reviewer
@Omega359Omega359Omega359 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

physical-exprChanges to the physical-expr crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@findepi@Omega359@alamb@comphead