Update RPAD scalar function to support Utf8View#11942
Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @Lordworms 🙏 . I had a few questions about this PR but it is looking close
| } | ||
| _ => Ok(None), | ||
| }) | ||
| .collect::<Result<GenericStringArray<T>>>() |
There was a problem hiding this comment.
This means the result of rpad is always GenericStringArray (Utf8Array) right?
I wonder if it would make sense to produce the same output type as the first argument 🤔 For the string view case eventually I could imagine reusing the same underlying buffers if the string had nothing to pad.
There was a problem hiding this comment.
lpad was the same here iirc
|
You may want to look at the tests I added in #11941 as an example to verify that all the signature variants are tested. |
I'll add that, thanks for reminding |
I may directly reference the test case there, is it ok? |
It currently hardcodes the LPadFunc in it so it'll require a refactor but given how close these two functions are I can't see an issue off the top of my head. |
| } | ||
| macro_rules! process_rpad { | ||
| // For the two-argument case | ||
| ($string_array:expr, $length_array:expr, $is_view:expr) => {{ |
There was a problem hiding this comment.
is_view looks to be unused and it always passed in true. Should be removed unless I missed something.
There was a problem hiding this comment.
I agree -- removing it would simplify this PR
There was a problem hiding this comment.
Sure, I'll remove it soon
|
Thank you @Lordworms and @Omega359 |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks good to me @Lordworms and @Omega359
An epic journey of macros 🚀
I can't help but think this function could be made much faster by using a StringBuilder rather than creating a bunch of Strings which are then copied.
However, that could be some future pR
@Omega359's PR here is a pretty good example of how to do this #11987 |
Sure, I'll add a PR for it tomorrow. |
|
Filed
Filed #11997 to track |
3 participants
Which issue does this PR close?
Closes #11918
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?