drusso commented on a change in pull request #8836:
URL: https://github.com/apache/arrow/pull/8836#discussion_r536885131
##########
File path: rust/datafusion/src/physical_plan/udf.rs
##########
@@ -56,6 +56,12 @@ impl Debug for ScalarUDF {
}
}
+impl PartialEq for ScalarUDF {
+ fn eq(&self, other: &Self) -> bool {
+ self.name == other.name && self.signature == other.signature
Review comment:
Thanks for pointing this out, I was afraid there might be an issue like
this.
I'm happy to back out the `PartialEq` derivation for `Expr` (as well as
`ScalarUDF` and `AggregateUDF`) , and do the equivalency checks manually in SQL
planner as needed.
Before I do that, I'm wondering if you think there is a more suitable
implementation of `PartialEq` for `ScalarUDF` and `AggregateUDF`? Maybe their
`eq()` implementations can:
1) Compare the function pointers?
2) Always return `false`?
I would advocate for some definition of equality for `Expr`, as, for
example, I imagine an optimizer (outside the context of the SQL planner) should
leverage equivalency of duplicated expressions. Of course equivalency doesn't
need to be via `PartialEq`, but that would be a nice convenience.
Let me know what you think. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
