-
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 concat, concat_ws, ends_with, initcap to datafusion-functions #10089
Conversation
Note that this PR isn't the final PR for removing the BuiltinScalarFunction - one last function (coalesce) remains which I'll create a ticket to migrate to the new crate shortly. |
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 -- I think this PR looks good. Thank you for pushing this over the line 🙏
@@ -843,18 +802,9 @@ impl WindowUDFImpl for SimpleWindowUDF { | |||
} | |||
} | |||
|
|||
/// Calls a named built in function | |||
pub fn call_fn(name: impl AsRef<str>, args: Vec<Expr>) -> Result<Expr> { |
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.
This is an interesting API change -- I wonder if this API is used to call functions by name in the expr API 🤔
However, it is currently going to be missing all the built in functions now that we port them to UDFs so a compiler error is probably better than a runtime error,
Maybe we can add such a function back eventually (using the API @jayzhan211 and @viirya were talking about in #9892 🤔 )
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 it's a useful function but from what I recall the compiler was complaining about it being unused currently. Of course, that doesn't mean it's not useful externally (it obviously is) but I think that re-implementing it in #9892 would be the best approach to take. I could add it back in during #10090 but I think it may have to live somewhere else unless a session context or something similar is passed in though
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.
it may have to live somewhere else unless a session context or something similar is passed in though
Yes I agree with this. Now that there is no static global list of functions some sort of state needs to get passed in
* chore: upgrade datafusion Deps Ref #690 * update concat and concat_ws to use datafusion_functions Moved in apache/datafusion#10089 * feat: upgrade functions.rs Upstream is continuing it's migration to UDFs. Ref apache/datafusion#10098 Ref apache/datafusion#10372 * fix ScalarUDF import * feat: remove deprecated suppors_filter_pushdown and impl supports_filters_pushdown Deprecated function removed in apache/datafusion#9923 * use `unnest_columns_with_options` instead of deprecated `unnest_column_with_option` * remove ScalarFunction wrappers These relied on upstream BuiltinScalarFunction, which are now removed. Ref apache/datafusion#10098 * update dataframe `test_describe` `null_count` was fixed upstream. Ref apache/datafusion#10260 * remove PyDFField and related methods DFField was removed upstream. Ref: apache/datafusion#9595 * bump `datafusion-python` package version to 38.0.0 * re-implement `PyExpr::column_name` The previous implementation relied on `DFField` which was removed upstream. Ref: apache/datafusion#9595
Which issue does this PR close?
Closes #9540
Rationale for this change
Builtin scalar function migration
What changes are included in this PR?
Code, tests.
Are these changes tested?
Yes
Are there any user-facing changes?
No