[GitHub] [arrow-datafusion] renato2099 commented on a diff in pull request #1510: Add factorial function

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

renato2099 commented on code in PR #1510:
URL: https://github.com/apache/arrow-datafusion/pull/1510#discussion_r841278823


##########
datafusion/src/physical_plan/functions.rs:
##########
@@ -546,6 +550,7 @@ pub fn return_type(
         BuiltinScalarFunction::Lpad => utf8_to_str_type(&input_expr_types[0], "lpad"),
         BuiltinScalarFunction::Ltrim => utf8_to_str_type(&input_expr_types[0], "ltrim"),
         BuiltinScalarFunction::MD5 => utf8_to_str_type(&input_expr_types[0], "md5"),
+        BuiltinScalarFunction::Factorial => Ok(DataType::Float64),

Review Comment:
   right but that is at a logical level, i.e., the `numeric` data type is not necessarily a physical data type. That said I am extremely not familiar with datafusion 😅 so if you could point me to some code, I'd appreciate it
   Also I took a quick look at PG and I think it converts `int64` to the actual data type (`numeric`) but maybe I also missed something in PG's code base 
   https://github.com/postgres/postgres/blob/46ab07ffda9d6c8e63360ded2d4568aa160a7700/src/backend/utils/adt/numeric.c#L3566



-- 
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: dev-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org