Jefffrey commented on code in PR #19911:
URL: https://github.com/apache/datafusion/pull/19911#discussion_r2710807939
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -223,13 +224,12 @@ impl ScalarUDFImpl for DateTruncFunc {
&self.signature
}
- // keep return_type implementation for information schema generation
- fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- if arg_types[1].is_null() {
Review Comment:
We need to move this logic into `return_field_from_args`, since currently it
relies on calling `return_type`
##########
datafusion/catalog/src/information_schema.rs:
##########
@@ -421,10 +421,22 @@ fn get_udf_args_and_return_types(
Ok(arg_types
.into_iter()
.map(|arg_types| {
- // only handle the function which implemented
[`ScalarUDFImpl::return_type`] method
+ // prefer the newer `return_field_from_args` API which allows
+ // nullability and value-based return type decisions. Build
+ // the expected `ReturnFieldArgs` from the example arg types
+ // and pass `None` for scalar arguments since we don't have
+ // concrete scalar values here.
Review Comment:
```suggestion
```
Personally I'd remove these comments; we don't need this much info, and some
of the info here can be misleading. For example it mentions using the
`return_field_from_args` API allows nullability & value-based return type
decisions, which is true, but not here; here we always set nullability as true
and set the scalar values as None, so they don't actually play a factor here.
This is a simple change which I don't think needs an explicit explanation,
same for below.
##########
datafusion/functions/src/datetime/date_trunc.rs:
##########
@@ -223,13 +224,12 @@ impl ScalarUDFImpl for DateTruncFunc {
&self.signature
}
- // keep return_type implementation for information schema generation
- fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- if arg_types[1].is_null() {
- Ok(Timestamp(Nanosecond, None))
- } else {
- Ok(arg_types[1].clone())
- }
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ // This UDF implements `return_field_from_args` and should use that
+ // for return type (including nullability) decisions. Returning an
+ // internal error here makes misuse visible and matches other UDFs
+ // that implement `return_field_from_args` directly.
Review Comment:
```suggestion
```
This comment isn't necessary, the error message is clear enough
--
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]
