fix: Support Utf8View in numeric_string_coercion
#13366
Conversation
jonathanc-n
changed the title
Utf8View in string_numeric_coercionUtf8View in string_numeric_coercion
| @@ -1490,6 +1491,92 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_numeric_string_coercion_with_utf8view() { | |||
| assert_eq!( | |||
There was a problem hiding this comment.
Can you find a test case that fail if the change is reverted instead of the rust test here?
There was a problem hiding this comment.
There are no test cases that will fail if changes are reverted other than this one.
There was a problem hiding this comment.
so it means the change is not solving the problem 🤔 maybe this function is not used anywhere?
There was a problem hiding this comment.
I'm a bit confused, the function is used here. If reverted, the change will fail the current added test but no other tests. This means that the changes allow support for Utf8View in numeric_string_conversion
There was a problem hiding this comment.
If I understand correctly, your test is unit test, it failed if the change reverted but it doesn't mean the change effects the actual query (I guess after we change the signature for most of the function, numeric string coercion might not be used at all). I suggest we add end2end test (slt) instead of unit test
There was a problem hiding this comment.
Ideally this query should pass after your change, but it didn't
SELECT make_array(arrow_cast('1.2', 'Utf8View'), 2.3);
Utf8View in string_numeric_coercionUtf8View in numeric_string_coercion
There was a problem hiding this comment.
I think this PR needs some end to end tests -- namely something like
> drop table foo;
0 row(s) fetched.
Elapsed 0.001 seconds.
> create table foo as values (arrow_cast('1', 'Utf8View')), (arrow_cast('2', 'Utf8View'));
0 row(s) fetched.
Elapsed 0.004 seconds.
> select column1 = 1 from foo;
Error during planning: Cannot infer common argument type for comparison operation Utf8View = Int64
|
@jonathanc-n do you have some time to add some SLT tests to this PR as suggested above? |
|
@alamb I believe this is waiting on the numeric to utf8view pr for arrow-rs. |
I think that means |
There was a problem hiding this comment.
Thank you @jonathanc-n -- I am trying to help get this feature through
I tried some more tests for this feature and they failed unfortunately
This is due to the fact stringview isn't yet supported for converting from numeric --> StringView
See
We either need to wait for that feature to get supported in arrow-rs or we would need to add a cast wrapper to workaround the gap (e.g. by casting first to StringArray and then to StringViewArray)
Which issue does this PR close?
Closes #13359 .
Rationale for this change
What changes are included in this PR?
Added
utf8viewtostring_numeric_coercionAre these changes tested?
Added tests for most numeric possibilities
Are there any user-facing changes?