universalmind303 commented on code in PR #8985:
URL: https://github.com/apache/arrow-datafusion/pull/8985#discussion_r1484906473


##########
datafusion/expr/src/udf.rs:
##########
@@ -249,6 +261,22 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
     /// the arguments
     fn return_type(&self, arg_types: &[DataType]) -> Result<DataType>;
 
+    /// What [`DataType`] will be returned by this function, given the types of
+    /// the expr arguments
+    fn return_type_from_exprs(

Review Comment:
   Hmm yeah @yyy1000 you still run into the same error then.
   
   
   
   I'm wondering if it'd be easiest to just change the type signature on 
`ExprSchemable` to be generic over the trait instead of the functions. 
   
   ```rs
   pub trait ExprSchemable<S: ExprSchema> {
       /// given a schema, return the type of the expr
       fn get_type(&self, schema: &S) -> Result<DataType>;
   
       /// given a schema, return the nullability of the expr
       fn nullable(&self, input_schema: &S) -> Result<bool>;
   
       /// given a schema, return the expr's optional metadata
       fn metadata(&self, schema: &S) -> Result<HashMap<String, String>>;
   
       /// convert to a field with respect to a schema
       fn to_field(&self, input_schema: &DFSchema) -> Result<DFField>;
   
       /// cast to a type with respect to a schema
       fn cast_to(self, cast_to_type: &DataType, schema: &S) -> Result<Expr>;
   }
   
   impl ExprSchemable<DFSchema> for Expr {
   //... 
   }
   ```
   
   
   then the trait can just go back to the original implementation you had using 
`&DFSchema`
   
   ```rs
       fn return_type_from_exprs(
           &self,
           arg_exprs: &[Expr],
           schema: &DFSchema,
       ) -> Result<DataType> {
           let arg_types = arg_exprs
               .iter()
               .map(|e| e.get_type(schema))
               .collect::<Result<Vec<_>>>()?;
           self.return_type(&arg_types)
       }
   ```
   
   I tried this locally and was able to get things to compile locally, and was 
able to implement a udf using the trait. 
   
   It does make it a little less flexible as it's expecting a `DFSchema`, but i 
_think_ thats ok? 
   
   I think the only other approach would be to make `ScalarUDFImpl` dynamic 
over `<S: ExprSchema>`, but I feel like that's much less ideal than just using 
a concrete type. 
   
   
   



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

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to