alamb commented on a change in pull request #307:
URL: https://github.com/apache/arrow-datafusion/pull/307#discussion_r630079422



##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -1373,20 +1374,28 @@ impl PhysicalExpr for ScalarFunctionExpr {
     }
 
     fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {

Review comment:
       I think this approach, while a hack, seems reasonable to me
   
   I agree with the comments on 
https://github.com/apache/arrow-datafusion/pull/303#discussion_r629859725 that 
this does seem like a hack (it would be nicer to have an enum or something that 
made this case explicit).
   
   
   I would like to see this expectation documented near the place where a user 
would define a UDF as well - either as an example or as a doccomment. Perhaps 
somewhere in 
   
https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/physical_plan/udf.rs
   
   Or bonus points for adding an example:
   
https://github.com/apache/arrow-datafusion/blob/master/datafusion-examples/examples/simple_udf.rs
   
   
   cc; @msathis  who I think faced a similar challenge in `now()`: 
https://github.com/apache/arrow-datafusion/pull/288




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


Reply via email to