[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8836: ARROW-10808: [Rust][DataFusion] Support nested expressions in aggregations.

[GitBox <gi...@apache.org>]

jorgecarleitao commented on a change in pull request #8836:
URL: https://github.com/apache/arrow/pull/8836#discussion_r536874229



##########
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:
       I am afraid that this may not be sufficient: two UDFs may have the same name and signature and represent different semantics. In general UDFs are anonymous functions and thus do not have a `partialEq`. If we base equality on `name` and two UDFs are assigned the same name, there will be a semantic error in this equality.
   
   Note that while this can't happen in SQL, because UDFs are registered via `register_udf(name, ...)`, the DataFrame API supports using bypassing the context and calling UDFs without registering them on the context.
   
   In the `simple_udf.rs` example, the line 
   
   ```rust
   let expr1 = pow.call(vec![col("a"), col("b")]);
   ```
   
   demonstrates how to use a UDF without registering it on the context, in which case its name is only used when printing the plan.




----------------------------------------------------------------
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:
users@infra.apache.org