Use return_field_from_args in information schema and date_trunc#20079
Use return_field_from_args in information schema and date_trunc#20079Jefffrey merged 4 commits intoapache:mainfrom
Conversation
github-actions
bot
added
catalog
| .into_iter() | ||
| .map(|arg_types| { | ||
| // only handle the function which implemented [`ScalarUDFImpl::return_type`] method | ||
| let arg_fields: Vec<FieldRef> = arg_types |
There was a problem hiding this comment.
This is interesting that we omitted window functions before 🤔
And somehow this fix doesn't affect any existing tests?
There was a problem hiding this comment.
Good morning!
If i'm allowed, this could use a dedicated test/tests, at the same time i wanted to keep It lean for reviewers.
If any follow up Is needed, please consider me for it
There was a problem hiding this comment.
I think this PR is fine to merge as is, but I wouldn't be opposed to adding some SLT tests for that case
| } | ||
|
|
||
| // keep return_type implementation for information schema generation | ||
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { |
There was a problem hiding this comment.
Please update https://github.com/AndreaBozzo/datafusion/blob/e8d78e81765137be6fd7258e84cdcd62fdddd907/datafusion/functions/benches/date_trunc.rs#L61 to use return_field_from_args
There was a problem hiding this comment.
thanks this should avoid internal_error, good catch
|
Thanks @AndreaBozzo & @martin-g |
3 participants
Which issue does this PR close?
return_field_from_args#19870.Rationale for this change
Some UDFs/UDAFs implement
return_field_from_args/return_fieldinstead ofreturn_type. The information schema was callingreturn_typedirectly, which fails for those functions. The default implementation ofreturn_field_from_argsalready delegates toreturn_type, so switching to the newer API works for all functions.What changes are included in this PR?
information_schema.rs:get_udf_args_and_return_typesnow callsreturn_field_from_argsinstead ofreturn_type;get_udaf_args_and_return_typesnow callsreturn_fieldinstead ofreturn_type. Removed stale comments referencing the old API.date_trunc.rs:return_typenow returnsinternal_err, andreturn_field_from_argsis self-contained (no longer delegates toreturn_type), following the same pattern as other UDFs likenamed_structandmap_from_arrays(ref: fix: derive custom nullability for sparkmap_from_arrays#19275).Are these changes tested?
Covered by existing information_schema sqllogictests and
datafusion-functionsunit tests.Are there any user-facing changes?
No.