-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implement native stringview support for BTRIM #11920
Conversation
Just an info: I tried combining the two functions |
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.
Thank you @Kev1n8 -- I think this PR is an improvement for sure.
} | ||
|
||
// removing 'a will cause compiler complaining lifetime of `func` | ||
fn string_view_trim<'a, T: OffsetSizeTrait>( |
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 implementation is an improvement -- however, it will now copy the string values to a new StringArray
I think if the function generated StringView output directly it would be possible to avoid all string copies (and only adjust the views).
I can file this as a follow on optimization idea.
I noticed this on #11790
Which issue does this PR close?
Closes #11835
Rationale for this change
What changes are included in this PR?
The changes mainly lie in datafusion/functions/string/common.rs, where I broke
generic_trim
up to deal with 2 conditions (whether to useUtf8View
or not). An additional parameter is also added to indicate the datatype. I also temporarily adjustedLTRIM
andRTRIM
to work through the process. If my implementation is considered a good practice, I would like to open follow-on PRs to haveLTRIM
andRTRIM
supportUtf8View
natively.And some tests were also added to string_view.slt covering the
Utf8View
support ofBTRIM
.Are these changes tested?
yes
Are there any user-facing changes?
no