Fix expr partial ord test

[mustafasrepo]

Which issue does this PR close?

Closes #.

Rationale for this change

test_partial_ord test, tests the partial_cmp implementation of datafusion_expr::expr::Expr enum. This implementation does comparison based on the hashes, hence only guarantee of this operator is comparisons are consistent. The comparison results may not be actual comparison result. This PR changes this misleading test to actually test, its behaviour.

Current test depends on hashing algorithm and its result possibly changes when hashing algorithm (or seed) changes. Actually when ahash crate version=0.8.7 is used this test fails.

What changes are included in this PR?

This PR fixes problem above.

Are these changes tested?

Are there any user-facing changes?

[comphead]

hm, but does that actually mean the comparison itself works not correctly sometimes?

[alamb]

I think the calculation is consistent (will always be either < or >)

However, I wonder if the hash values are always consistent (like perhaps do they vary on x86_64 and M1 platforms?) Does that matter 🤔

[tustvold]

Ahash is not a stable hash algorithm and may not only change between platforms, but also minor releases. I lack context into why we are ordering based on hashes, but my initial response is this is probably incorrect, especially if it is inconsistent with the equality relation.

[alamb]

but my initial response is this is probably incorrect, especially if it is inconsistent with the equality relation.

Why is it "inconsistent with equality" ? If two exprs are equal, they would have the same hash, so the order between hash values would be consistent (on a certain platform and release)

That is not to say we shouldn't change how ordering is done, but I just don't understand this comment

[tustvold]

I haven't checked this, but if the PartialEq implementation isn't also using this hash approach, the two would be inconsistent

Edit: as suspected it looks like PartialEq is being derived using a proc macro, and is therefore inconsistent with this implementation of PartialOrd - https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr.rs#L87. In particular PartialOrd could claim things to be equal due to a hash collision, when PartialEq would indicate they are not

[alamb]

Thank you @mustafasrepo and everyone

Note we saw failures of this test while verifying the 35.0.0 release: https://lists.apache.org/thread/onbs8l0w5s7693fchpyvwwgh61gf1jf8

I believe we should merge this PR as it is a test only change and it will make the test match what the code is doing

However, as has been mentioned, comparing hashes is probably not a good idea in general so I filed #8932 to track improving PartialOrd

BTW if anyone has a practical reason / example why comparing hashes is bad (e.g. bugs it could cause or other issues observable by customers) please add them to #8932 as that will help prioritize getting it done

[mustafasrepo]

I believe we should merge this PR as it is a test only change and it will make the test match what the code is doing

I aggree with you in this regard. I will go ahead an merge this. It is really weird PartialOrd is implemented this way. However, we can continue discussion about how to proceed (whether to change it, to remove it, etc) with existing implementation in #8932