Repository navigation
Incorrect implementation of PartialOrd for ScalarUDF, WindowUDF and AggregateUDF #17064
Description
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 == bif and only ifpartial_cmp(a, b) == Some(Equal).
[...]
thus the contract is violated
We should remove these incorrect PartialOrd implementations.
Activity
findepi commentedon Aug 8, 2025
Removing the
PartialOrdwould be the best way to go.This would probably mean removing PartialOrd from Expr, perhaps reverting
cc @ngli-me @jayzhan211 @alamb
alamb commentedon Aug 12, 2025
Getting it consistent would be very good, thank you @findepi
findepi commentedon Aug 12, 2025
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
PartialOrdinstead.Why do we have ordering?
alamb commentedon Aug 13, 2025
I think it was used to generate a consistent canonical version of an expression
For example to ensure that
IN (x, y, z)andIN(y, z, x)were the same, and that the join predicates are normalized or somethingI can't remember the specifics though
findepi commentedon Aug 13, 2025
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
alamb commentedon Sep 10, 2025
🎉