[GitHub] [arrow-datafusion] alamb commented on a change in pull request #307: fix 305 by using a scalar uint as param for zero param functions

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

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