Update REVERSE scalar function to support Utf8View #11973
Conversation
| @@ -83,10 +85,17 @@ impl ScalarUDFImpl for ReverseFunc { | |||
| /// reverse('abcde') = 'edcba' | |||
| /// The implementation uses UTF-8 code points as characters | |||
| pub fn reverse<T: OffsetSizeTrait>(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
| let string_array = as_generic_string_array::<T>(&args[0])?; | |||
| if args[0].data_type() == &Utf8View { | |||
There was a problem hiding this comment.
probably we can keep same pattern like in invoke ? it would be more readable
There was a problem hiding this comment.
as in using a match? Sure. It won't be the same match arms though.
|
|
||
| use datafusion_common::{Result, ScalarValue}; | ||
| use datafusion_expr::{ColumnarValue, ScalarUDFImpl}; | ||
|
|
||
| use crate::unicode::reverse::ReverseFunc; | ||
| use crate::utils::test::test_function; | ||
|
|
||
| macro_rules! test_reverse { |
There was a problem hiding this comment.
is it possible to have a function rather than macro?
There was a problem hiding this comment.
Could, sure. I'm curious as to why though. This is just the same approach as I used for lpad.
There was a problem hiding this comment.
yeah this testing pattern is somewhat common in the string functions
I happen to like .slt in general, but I know that is not for everyone
There was a problem hiding this comment.
I agree, we trying to use macros if other options didn't work. It takes more time when people debug tests to expand the macros and go through them.
if its already used in string tests its okay, perhaps we want to revisit macros and replace them with funcs whenever possible
There was a problem hiding this comment.
Great explanation ,thanks.
|
|
||
| use datafusion_common::{Result, ScalarValue}; | ||
| use datafusion_expr::{ColumnarValue, ScalarUDFImpl}; | ||
|
|
||
| use crate::unicode::reverse::ReverseFunc; | ||
| use crate::utils::test::test_function; | ||
|
|
||
| macro_rules! test_reverse { |
There was a problem hiding this comment.
yeah this testing pattern is somewhat common in the string functions
I happen to like .slt in general, but I know that is not for everyone
Which issue does this PR close?
Closes #11915
Rationale for this change
Update reverse udf to support utf8view
What changes are included in this PR?
code, tests
Are these changes tested?
Yes
Are there any user-facing changes?
No