[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8080: ARROW-9887: [Rust] [DataFusion] Added support for complex return types for built-in functions

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

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



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -592,7 +607,7 @@ mod tests {
     fn select_scalar_func_with_literal_no_relation() {
         quick_test(
             "SELECT sqrt(9)",
-            "Projection: sqrt(CAST(Int64(9) AS Float64))\
+            "Projection: sqrt(Int64(9))\

Review comment:
       It depends where want to perform the cast. This test was ensuring that the cast was happenning on the logical plane, due to the type coercer.
   
   IMO `sqrt(8)` is a logical expression that everyone agrees should return `2.82842712475...` and the reason we cast it to f64 is entirely due to a limitation of our physical expressions, that only implement `sqrt` for f64.
   
   For this reason, IMO these type of casting operations should only be done on our physical plane. For example, a different physical planner could prefer to not have it `cast`ed because it has a specialized implementation that supports other types.
   
   This PR moves casting operations on built-in functions to the physical plane, like we are performing for binary operators.
   
   Note that the user continues to have full transparency: they can use `EXPLAIN verbose` to get the physical plan, that contains the cast.




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