-
Notifications
You must be signed in to change notification settings - Fork 78
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
feat: add python bindings for ends_with function #693
Conversation
1dff1fa
to
4d37a39
Compare
Looks like I accidentally deleted a line i changed on my rebase. added back the wrap pyfunction. |
src/functions.rs
Outdated
@@ -474,6 +474,7 @@ expr_fn!( | |||
); | |||
expr_fn!(sqrt, num); | |||
expr_fn!(starts_with, arg1 arg2, "Returns true if string starts with prefix."); | |||
expr_fn!(ends_with, arg1 arg2, "Returns true if string ends with suffix."); |
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.
We should probably match the argument names to upstream: string suffix
(I assume you just matched what starts_with
has. That was either my error as I made the initial port or the upstream UDF changed)
https://docs.rs/datafusion/latest/datafusion/functions/expr_fn/fn.ends_with.html
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.
updated! Also updated the argument names for starts_with
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.
actually...looks like the upstream for starts_with is arg1
, and arg2
...should i revert it back?
https://docs.rs/datafusion/latest/datafusion/functions/expr_fn/fn.starts_with.html
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.
:D yea - UDF's are going through rapid iteration in upstream datafusion
, so the conventions are still being ironed out.
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'm fine with clarifying argument names for starts_with
. I assume upstream will get switched to this eventually.
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 @richtia. There is a merge conflict that needs fixing though.
32983ec
to
ada9d18
Compare
Just rebased |
Which issue does this PR close?
ends_with function is missing from datafusion_python