BUG: pandas.Series.unique() does not return correct unique values on non UTF8 enodeable strings #58215
Conversation
When processing an invalid Unicode string, the exception handler for UnicodeEncodeError called `get_c_string` with an ephemeral repr value that could be garbage-collected the next time an exception was raised. Issue pandas-dev#45929 demonstrates the problem. This commit fixes the problem by retaining a Python reference to the repr value that underlies the C string until after all `values` are processed. Wisdom from StackOverflow suggests that there's very small performance difference between pre-allocating the array vs. append if indeed we do need to fill it all the way, but because we only need references on exceptions, we expect that in the usual case we will append very few elements, making it faster than pre-allocation. Signed-off-by: Michael Tiemann <[email protected]>
The `single_cpu` attribute for `test_unique_bad_unicode` was likely an attempt to cover over the underlying bug fixed with this commit. We can now run this test in the usual fashion. Added a test case for the problem reported in 45929. Signed-off-by: Michael Tiemann <[email protected]>
Correctly alphabetize items in `Other` list. Signed-off-by: Michael Tiemann <[email protected]>
mroeschke
changed the title
| @@ -1179,6 +1179,8 @@ cdef class StringHashTable(HashTable): | |||
| use_na_value = na_value is not None | |||
|
|
|||
| # assign pointers and pre-filter out missing (if ignore_na) | |||
| # https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#caveats-when-using-a-python-string-in-a-c-context | |||
| keep_bad_unicode_refs = [] | |||
There was a problem hiding this comment.
This doesn't fix the underlying issue. It may make it less likely but the lifecycle is still not properly controlled
I think what needs to be done is something akin to:
'1 \udcd6a NY'.encode('utf8', errors="surrogatepass").decode('utf8', errors="surrogatepass")I think you can just do that directly in Cython, but we can look at the underlying C API if needed
(side note - I would really love to get rid of get_c_string - not a value added layer of indirection)
There was a problem hiding this comment.
I was able to replace get_c_string with the associated C APIs, but if I understand https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#caveats-when-using-a-python-string-in-a-c-context correctly, since we're storing char pointers from these encoded python strings and using them later, we need to keep a reference to those Python strings
There was a problem hiding this comment.
Oh OK I see the difference - CPython can cache the UTF8 bytes of a string alongside the string object. When the string is not utf8 encodable and we have to create temporary objects we run into trouble. So the list here artificially extends the lifetime of new objects guaranteed to be utf8 encodable to rely on that caching mechanism
To be honest I would be -1 on this change and would rather call it a wontfix. It is a very niche issue that fights the internals of CPython (and for that matter pyarrow, whose strings are utf8).
If someone wanted mixed-encoding Python strings like this I think pa.binary() is a better data type choice
There was a problem hiding this comment.
OK I'll close this PR then. My main motivation was to make test_unique_bad_unicode not flaky due to this issue, but I'll open a follow up PR making this test permanentlyxfail(strict=True)instead
There was a problem hiding this comment.
I think the name test_unique_bad_unicode is a misnomer. The code point "\ud83d" exists in Unicode. It is a high surrogate
https://www.unicode.org/charts/PDF/UD800.pdf
The problem is that by itself that high surrogate doesn't mean anything (it would need to be paired with a low surrogate). As such, it doesn't represent any glyph in any encoding.
AFAIU if you wanted to keep that unicode code point, you would have to:
- Convert the Python Unicode object to bytes via
str.encode(<encoding>, errors="surrogatepass") - Run your algorithms against the surrogatepass bytes
- Convert your surrogatepassed bytes back to a unicode string via `bytes.decode(, errors="surrogatepass")
I realize step 2 may not exist today but is a good impetus to work on interop with pa.binary() if this is required
Revival of #55530