feat: Support Utf8View for get_wider_type + binary_to_string_coercion functions
#13370
Conversation
|
Same with this, I would expect test that failed if the change is reverted |
| @@ -1550,6 +1552,62 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_get_wider_type_with_utf8view() { | |||
| assert_eq!( | |||
There was a problem hiding this comment.
Not this kind of test, but a valid sql query that goes through the coercion function
There was a problem hiding this comment.
@jayzhan211 Correct me if I'm wrong, but this seems to be a problem with arrow casting from numerics to utf8view, the same can be said for this, #13366. However correct me if I'm wrong, I can't seem to find a sql query that triggers it, although i believe there should be one
There was a problem hiding this comment.
It seems only like_coercion utilize this coercion. I think current coercion is unnecessary complex for like operator
There was a problem hiding this comment.
I think get_wider_type is actually only used by ArrayConcat
There was a problem hiding this comment.
I mean binary_to_string_coercion
| ); | ||
| assert_eq!( | ||
| binary_to_string_coercion(&DataType::LargeBinary, &DataType::Utf8View), | ||
| Some(DataType::Utf8View) |
There was a problem hiding this comment.
this is incorrect, it should be LargeUtf8 I think.
| (LargeBinary, Utf8) => Some(LargeUtf8), | ||
| (LargeBinary, LargeUtf8) => Some(LargeUtf8), | ||
| (LargeBinary, Utf8View) => Some(Utf8View), |
There was a problem hiding this comment.
This is incorrect, it should be LargeUtf8
There was a problem hiding this comment.
Beyond the one bug I noted this looks fine to me. I do think however there should be a ticket filed to look at the get_wider_type function and see if it can be removed altogether as there seems to be just one use case and this may (I haven't checked but I feel it's likely) duplicate a function in arrow-cast.
Which issue does this PR close?
Closes #13361
Closes #13360.
Rationale for this change
What changes are included in this PR?
Added string view to functions
Are these changes tested?
Are there any user-facing changes?