move Trunc, Cot, Round, iszero functions to datafusion-functions #10000
Conversation
github-actions
bot
added
sql
| let mut decimal_places = ColumnarValue::Scalar(ScalarValue::Int64(Some(0))); | ||
|
|
||
| if args.len() == 2 { | ||
| decimal_places = ColumnarValue::Array(args[1].clone()); |
There was a problem hiding this comment.
The whole ColumnarArray -> ArrayRef -> ColumnarArray in this code really should be cleaned up in another PR imho.
There was a problem hiding this comment.
yes, I agree.
These are some of the oldest functions in DataFusion so were written at a very different time. We have much better patterns now
| (floor, num, "nearest integer less than or equal to argument"), | ||
| (pi, , "Returns an approximate value of π") | ||
| ); | ||
| /// Return a list of all functions in this package |
There was a problem hiding this comment.
I had to replace the export_functions macro usage here because it doesn't handle Vec args such as is used by trunc function.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you @Omega359 .
There was some pretty gnarly merge conflicts to handle due to #9983
I did this locally as part of my review and made a PR to your branch Omega359#2 in case you would ilke to use that
Thanks again
| let mut decimal_places = ColumnarValue::Scalar(ScalarValue::Int64(Some(0))); | ||
|
|
||
| if args.len() == 2 { | ||
| decimal_places = ColumnarValue::Array(args[1].clone()); |
There was a problem hiding this comment.
yes, I agree.
These are some of the oldest functions in DataFusion so were written at a very different time. We have much better patterns now
| // 12 was Log10 | ||
| // 13 was Log2 | ||
| Round = 14; | ||
| // 14 was Round |
There was a problem hiding this comment.
Wow -- this list is looking pretty small now 🙏
Yeah, I was expecting that depending on which landed first. I'll check out your merge tonight - thanks! |
Alamb/resolve conflict
|
Check config failure seems unrelated: https://github.com/apache/arrow-datafusion/actions/runs/8606494312/job/23585168125?pr=10000
I'll try and rerun it when the rest of the tests pass |
Thanks, all the tests I have are passing locally. |
|
Reopened restart checks |
|
Just let me know when you are happy with this PR @Omega359 and I'll merge it in |
…l ordering theme.
|
I believe this PR is complete and ready to merge @alamb, thx for the review and handling the merge conflict. |
|
Thanks again @Omega359 |
2 participants
Which issue does this PR close?
Closes #9859
Rationale for this change
Function migration for Trunc, cot, round and iszero functions as part of #9285,
What changes are included in this PR?
Code, tests. math/mod.rs had to be redone to account for trunc taking a Vec argument.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.