[GitHub] [arrow] alamb commented on a change in pull request #8032: ARROW-9836: [Rust][DataFusion] Improve API for usage of UDFs

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

alamb commented on a change in pull request #8032:
URL: https://github.com/apache/arrow/pull/8032#discussion_r476466084



##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -193,6 +191,17 @@ impl ExecutionContext {
         state.scalar_functions.insert(f.name.clone(), Arc::new(f));
     }
 
+    /// Get a reference to the registered scalar functions
+    pub fn scalar_functions(&self) -> Vec<String> {

Review comment:
       This is not returning a reference, it is returning a copy 

##########
File path: rust/datafusion/src/logicalplan.rs
##########
@@ -1042,6 +998,35 @@ pub fn can_coerce_from(type_into: &DataType, type_from: &DataType) -> bool {
     }
 }
 
+/// A registry of functions used to plan queries programmatically

Review comment:
       I wonder what benefit we get from having Registry be its own `struct` and copying the vec around.
   
   What do you think about something like using a traint. Something like 
   
   ```
   pub trait UDFFactory {
       pub fn udf(&self, name: &str, args: Vec<Expr>) -> Result<Expr>;
   }
   ```
   
   And then move the implementation of `udf` directly into ExecutionContext. 
   
   
   
   
   

##########
File path: rust/datafusion/src/logicalplan.rs
##########
@@ -260,12 +261,10 @@ pub enum Expr {
     },
     /// scalar function
     ScalarFunction {
-        /// Name of the function
-        name: String,
+        /// The function
+        fun: Box<ScalarFunction>,

Review comment:
       ```suggestion
           fun: Arc<ScalarFunction>,
   ```
   
   I think you can avoid deep copying the `ScalarFunction`  if we use `Arc` consistently

##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -527,9 +527,8 @@ impl<'a, S: SchemaProvider> SqlToRel<'a, S> {
                             }
 
                             Ok(Expr::ScalarFunction {
-                                name: name.clone(),
+                                fun: Box::new(fm.as_ref().clone()),

Review comment:
       I think if `fun` was an `Arc` this line will look like `fun: fm.clone()`




----------------------------------------------------------------
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