-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move Floor, Gcd, Lcm, Pi to datafusion-functions #9976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is behavior change
, so I add label.
As part of #9285 (for link EPIC issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Omega359
Thanks @jackwener - may I ask what you see as a behaviour change? From my understanding there shouldn't be any user facing changes beyond where a function is located in the code (for dataframe use cases). Did I miss/change something inadvertently (it's happened before ...) ? |
use crate::utils::make_scalar_function; | ||
|
||
#[derive(Debug)] | ||
pub struct LcmFunc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it bepub(super)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so - this is following the pattern set out in #9903
my mistake, sorry for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prepare to merge this PR next day if others don't have any question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks @Omega359
Will merge it before the conflicts |
Which issue does this PR close?
Closes #9861
Rationale for this change
Function migration.
What changes are included in this PR?
code, tests.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.