information_schema: use return_field_from_args for scalar UDFs #19911
Conversation
|
I’ve completed the changes for issue #19870 (information_schema: use return_field_from_args). All local checks (cargo check and cargo clippy) pass ✅ Changes are tested and ready for review Could you please: Review and approve the PR Approve the pending workflows so CI can run fully Thanks a lot for your time! |
…hema; functions: implement return_field_from_args for date_trunc
Phaneendra293
force-pushed
the
issue-19870-return-field
branch
from
04b638b to
176d742
Compare
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
Thanks for picking this up.
Could you please make sure the CI checks are passing locally before pushing for review?
|
|
||
| // keep return_type implementation for information schema generation | ||
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { | ||
| if arg_types[1].is_null() { |
There was a problem hiding this comment.
We need to move this logic into return_field_from_args, since currently it relies on calling return_type
There was a problem hiding this comment.
Again, this comment was resolved without any action taken.
| // 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. |
There was a problem hiding this comment.
| // 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. |
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.
There was a problem hiding this comment.
Please refrain from resolving a conversation if no action has been taken nor any comment left to explain why action was not taken.
|
Thanks for the review. I’ve run the CI checks locally: "cargo check -p datafusion-functions" "cargo clippy -p datafusion-functions --all-targets --all-features" "cargo test -p datafusion-functions" All checks are passing locally |
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
I would recommend running
cargo fmt
cargo testlocally. We can't assume changes in one crate will affect only that crate; we should run the suite for the whole project.
| // 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. |
There was a problem hiding this comment.
Please refrain from resolving a conversation if no action has been taken nor any comment left to explain why action was not taken.
| // KIND, either express or implied. See the License for the | ||
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| use datafusion_common::internal_err; |
There was a problem hiding this comment.
Move this import down, it's causing the license CI check to fail
|
|
||
| // keep return_type implementation for information schema generation | ||
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { | ||
| if arg_types[1].is_null() { |
There was a problem hiding this comment.
Again, this comment was resolved without any action taken.
|
Hi, I wanted to share an update. I tried building datafusion-functions-aggregate-common to work on date_trunc.rs, but due to other pressing commitments and the amount of work I currently have, I won’t be able to continue with this task for the time being. I truly appreciate the opportunity to contribute and your understanding regarding this. At this point, I won’t be able to give the time and attention this task deserves. Hopefully, when my schedule allows, I can return and contribute again in the future. Thank you for your support and guidance. |
2 participants
This PR updates information_schema to derive ScalarUDF return types using return_field_from_args.
Ensures correct handling of nullability and value-dependent return types.
Fixes #19870