Move starts_with, to_hex, trim, upper to datafusion-functions (and add string_expressions)
#9541
Conversation
There was a problem hiding this comment.
Thanks @Tangruilin -- this is looking good.
The PR title says it moves 4 functions but the PR only moves one function (starts_with) -- this is good, but I think the title should be updated to reflect the change.
I think we also need to remove the existing BuiltInScalarFunction::StartsWith variant in the same PR
|
|
||
| #[cfg(test)] | ||
| mod tests {} |
There was a problem hiding this comment.
I don't think we need an empty test module, do we?
| #[cfg(test)] | |
| mod tests {} |
There was a problem hiding this comment.
I don't think we need an empty test module, do we?
Sorry... This PR is only a draft now
github-actions
bot
added
logical-expr
Tangruilin
force-pushed
the
task#9539
branch
3 times, most recently
from
de09b18 to
0171242
Compare
|
Hi @Tangruilin -- I wonder how this PR is going? |
I need to slove the conflicts, then the PR can be reviewed This will be finished today |
…n-functions Signed-off-by: tangruilin <[email protected]>
alamb
changed the title
starts_with, to_hex, trim, upper to datafusion-functions (and add string_expressions)
datafusion/functions/Cargo.toml
Outdated
| default = [ | ||
| "core_expressions", | ||
| "datetime_expressions", | ||
| "encoding_expressions", | ||
| "math_expressions", | ||
| "regex_expressions", | ||
| "crypto_expressions", | ||
| ] | ||
| "string_expressions", | ||
| ] # Enable encoding by default so the doctests work. In general don't automatically enable all packages. |
There was a problem hiding this comment.
It seems strange the comment was moved down to the bottom. Shouldn't it still be at the top?
| @@ -650,26 +650,6 @@ async fn test_fn_split_part() -> Result<()> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
There was a problem hiding this comment.
I don't understand why these tests were removed. I think it is important that we can still use these functions with the dataframe API.
There was a problem hiding this comment.
I poked around and this seems like it was because the string_functions expr_fn weren't exported. I took the liberty of fixing this in b803062
|
Thanks again @Tangruilin |
…tions
Which issue does this PR close?
Closes #9539 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?