Support upper and lower in strings_udf - #12099
Conversation
Codecov ReportBase: 87.47% // Head: 88.24% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #12099 +/- ##
================================================
+ Coverage 87.47% 88.24% +0.77%
================================================
Files 133 137 +4
Lines 21826 22553 +727
================================================
+ Hits 19093 19903 +810
+ Misses 2733 2650 -83
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| std::int64_t flags_table, | ||
| std::int64_t cases_table, | ||
| std::int64_t special_table) |
There was a problem hiding this comment.
Just curious why these are not void* as well?
There was a problem hiding this comment.
Let me make sure all is right here. Each of the functions that return a pointer to an input mapping table returns its own type of pointer. For the flags table its uint8_t*, for the cases table its uint16_t. For the special case mapping table it's a special_case_mapping*. Would the correct thing to do in this case be to receive each of these as a uintptr_t in the cython and then carry them through the python into the lowering as a np.uintp? Then these shim functions could accept a uintptr_t here.
There was a problem hiding this comment.
I think it would be more correct for these to be pointer types than int64_t types.
There was a problem hiding this comment.
If you can interact with ctypes I think you can carry around a ctypes.cvoidp.
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
A bunch of nits around cross-calling ABI and type-punning pointers and integers.
|
|
||
| extern "C" __device__ int lower(int* nb_retval, | ||
| void* udf_str, | ||
| void* const* st, |
There was a problem hiding this comment.
Does this need to be a void ** or can it just be a void * (I note that inside you cast to string_view * and then dereference, so I think you can strip a * everywhere).
There was a problem hiding this comment.
I am not sure I follow here. This follows the pattern from the rest of the shim functions when a string_view is an arg. IIUC st is pointing directly to the struct, so only one level of pointing right?
There was a problem hiding this comment.
Most functions here (e.g. https://github.com/rapidsai/cudf/blob/418cbc69b336f34b06e1de21ae2c016b40f1e6fc/python/strings_udf/cpp/src/strings/udf/shim.cu#L186-L193) that take a pointer to string_view take the argument as void const *, not void* const *. Why is this different?
There was a problem hiding this comment.
ah you're right! updated this. Nice catch
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
|
rerun tests |
galipremsagar
added
5 - Ready to Merge
|
some test failures here that seem to be related to a previous PR that I am looking into now. |
|
I am thinking these CI failures might be transient. Going to |
|
rerun tests |
|
@gpucibot merge |
4 participants
This PR adds support for the following two functions in
strings_udf:str.upper()str.lower()Part of #9639