martin-g commented on code in PR #19245: URL: https://github.com/apache/datafusion/pull/19245#discussion_r2606581710
########## datafusion/spark/src/function/string/format_string.rs: ########## Review Comment: Other functions that implement `return_field_from_args()` return an internal error here. See https://github.com/apache/datafusion/pull/19232/files#diff-ccded5b9988cccba279238a5300620d4803770d28fa2909b5388bd5009bbeeffR64 You can move the body to `return_field_from_args()`. ########## datafusion/spark/src/function/string/format_string.rs: ########## @@ -86,6 +86,17 @@ impl ScalarUDFImpl for FormatStringFunc { } } + fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> { + let arg_types: Vec<DataType> = args + .arg_fields + .iter() + .map(|f| f.data_type().clone()) + .collect(); + let data_type = self.return_type(&arg_types)?; + let nullable = args.arg_fields.iter().any(|f| f.is_nullable()); Review Comment: ``` spark-sql (default)> SELECT format_string(NULL, NULL); NULL Time taken: 0.044 seconds, Fetched 1 row(s) spark-sql (default)> SELECT typeof(format_string(NULL, NULL)); string Time taken: 0.032 seconds, Fetched 1 row(s) ``` This is more interesting! If the first argument is NULL then Spark returns `NULL` (not `null`), but the type is still `string`. ########## datafusion/spark/src/function/string/format_string.rs: ########## @@ -86,6 +86,17 @@ impl ScalarUDFImpl for FormatStringFunc { } } + fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> { + let arg_types: Vec<DataType> = args + .arg_fields + .iter() + .map(|f| f.data_type().clone()) + .collect(); + let data_type = self.return_type(&arg_types)?; + let nullable = args.arg_fields.iter().any(|f| f.is_nullable()); Review Comment: This is not quite correct! Search for `self.format_string(string, "null")` in this file. Nulls are printed as `"null"`, so the result is non-nullable. ########## datafusion/spark/src/function/string/format_string.rs: ########## @@ -86,6 +86,17 @@ impl ScalarUDFImpl for FormatStringFunc { } } + fn return_field_from_args(&self, args: ReturnFieldArgs) -> Result<FieldRef> { + let arg_types: Vec<DataType> = args + .arg_fields + .iter() + .map(|f| f.data_type().clone()) + .collect(); + let data_type = self.return_type(&arg_types)?; + let nullable = args.arg_fields.iter().any(|f| f.is_nullable()); Review Comment: Spark also returns a String: ``` spark-sql (default)> SELECT format_string("%d", NULL); null Time taken: 0.98 seconds, Fetched 1 row(s) spark-sql (default)> SELECT typeof(format_string("%d", NULL)); string Time taken: 0.035 seconds, Fetched 1 row(s) ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
