Add PartialOrd for the DF subfields/structs for the WindowFunction expr

[ngli-me]

Which issue does this PR close?

Related #8932 (well, part of it).

Rationale for this change

Smaller PR for changing the Datafusion side subfields/structs relating to the WindowFunction expression type. There are still a few Arrow types that need PartialOrd implementations, looking for some direction on whether those should be added, or if the comparison for those fields can be ignored.

What changes are included in this PR?

BuiltInWindowFunction -> Derives PartialOrd
AggregateUDFImpl -> Manual implementation of PartialOrd based on name + signature fields
AggergateUDF -> Derives PartialOrd from impl (Added tests for this ordering as well)
WindowUDFImpl -> Manual implementation of PartialOrd based on name + signature fields
WindowUDF -> Derives PartialOrd from impl (Added tests for this ordering as well)
TypeSignature -> Derives PartialOrd (Added some sanity assertions for this)

Are these changes tested?

Yes, but not extensively.

Are there any user-facing changes?

Some of the subtypes will have PartialOrd applicable.

[alamb]

Thank you @ngli-me -- this looks like a very nice incremental step. I have a question about manually implementing PartialOrd but otherwise this is 👌

[alamb]

I think we could automatically derive PartialOrd for these enums rather than implement them manually.

using derive I think would be easier to maintain over the long term (aka it is easier to add new variants to the enum without having ti update the partial ord implemetnation)

I tried this locally and it seems to work:

andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ git diff
diff --git a/datafusion/expr-common/src/signature.rs b/datafusion/expr-common/src/signature.rs
index 2043757a4..3f0f61f2f 100644
--- a/datafusion/expr-common/src/signature.rs
+++ b/datafusion/expr-common/src/signature.rs
@@ -84,7 +84,7 @@ pub enum Volatility {
 ///   DataType::Timestamp(TimeUnit::Nanosecond, Some(TIMEZONE_WILDCARD.into())),
 /// ]);
 /// ```
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd)]
 pub enum TypeSignature {
     /// One or more arguments of an common type out of a list of valid types.
     ///
@@ -127,7 +127,7 @@ pub enum TypeSignature {
     Numeric(usize),
 }

-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd)]
 pub enum ArrayFunctionSignature {
     /// Specialized Signature for ArrayAppend and similar functions
     /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.

[ngli-me]

Ah, for some reason I thought that derives wouldn't work with an enum with differing types, but I see it does work with the tests. Added an additional sanity assertion just to check, and removed the manual implementation for TypeSignature, thanks!

[alamb]

Looks good to me -- thank you @ngli-me

[alamb]

(I think the description of this PR might need to be udpated)

[alamb]

Also, there appears to be a clippy failure

[ngli-me]

Made some adjustments to fix that clippy issue and fmt.

[alamb]

🚀
THanks again @ngli-me

[jayzhan211]

Can we not implement PartialOrd for function? I don't think it make sense to have ordering concept for the function.
If there is some reason that we can't avoid it, can we compare with the name only? When I try to implement LogicalType for TypeSignature, I forced to implement PartialOrd as well, but I don't think it make sense

[alamb]

If there is some reason that we can't avoid it, can we compare with the name only?

I think one place it is used is to normalize the order of expressions (maybe for CSE or simplification 🤔 )

Comparing on name is probably fine too I would think