Incorrect implementation of `PartialOrd` for `ScalarUDF`, `WindowUDF` and `AggregateUDF`

[findepi]

PartialOrd implementation for ScalarUDF, WindowUDF and AggregateUDF compares name and signature only.
PartialEq implementation for these structs delegates to the function implementation and thus may compare other properties.

the contract for these traits is (https://doc.rust-lang.org/std/cmp/trait.PartialOrd.html):

a == b if and only if partial_cmp(a, b) == Some(Equal).
[...]

thus the contract is violated

We should remove these incorrect PartialOrd implementations.

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

[findepi]

Removing the PartialOrd would be the best way to go.
This would probably mean removing PartialOrd from Expr, perhaps reverting

• Add PartialOrd for the DF subfields/structs for the WindowFunction expr #12421
• Implement PartialOrd for Expr and sub fields/structs without using hash values #12481

cc @ngli-me @jayzhan211 @alamb

[alamb]

Getting it consistent would be very good, thank you @findepi

[findepi]

Getting it consistent would require delegating to UDF impl, just like we do with Eq and Hash in #16677. (and thus a breaking change).
Echoing @jayzhan211 's suggestion #12421 (comment), I'd like to understand whether we can remove PartialOrd instead.
Why do we have ordering?

[alamb]

Why do we have ordering?

I think it was used to generate a consistent canonical version of an expression

For example to ensure that IN (x, y, z) and IN(y, z, x) were the same, and that the join predicates are normalized or something

I can't remember the specifics though

[findepi]

For this having partial ordering, or ordering by a partial projection from an Expr, should be sufficient.

However, there remains a problem of discrepancy between PartialOrd and PartialEq/Eq impls.
We can fix the PartialOrd implementation by consulting Eq. Maybe something like

• Fix PartialOrd for ScalarUDF #17182