From c300b46e33be8b839937bd2cae96a2d3846d4fa0 Mon Sep 17 00:00:00 2001 From: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com> Date: Sun, 15 Oct 2023 08:15:19 -0400 Subject: [PATCH 1/3] Resolve Heisenbug in StringHashTable._unique 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 #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 <72577720+MichaelTiemannOSC@users.noreply.github.com> --- doc/source/whatsnew/v2.2.0.rst | 1 + pandas/_libs/hashtable_class_helper.pxi.in | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v2.2.0.rst b/doc/source/whatsnew/v2.2.0.rst index ef59c86a21598..bf396b8124136 100644 --- a/doc/source/whatsnew/v2.2.0.rst +++ b/doc/source/whatsnew/v2.2.0.rst @@ -403,6 +403,7 @@ Other ^^^^^ - Bug in :func:`cut` incorrectly allowing cutting of timezone-aware datetimes with timezone-naive bins (:issue:`54964`) - Bug in :meth:`DataFrame.apply` where passing ``raw=True`` ignored ``args`` passed to the applied function (:issue:`55009`) +- Bug in Cython :meth:`StringHashTable._unique` used ephemeral repr values when UnicodeEncodeError was raised (:issue:`45929`) - Bug in rendering ``inf`` values inside a a :class:`DataFrame` with the ``use_inf_as_na`` option enabled (:issue:`55483`) - Bug in rendering a :class:`Series` with a :class:`MultiIndex` when one of the index level's names is 0 not having that name displayed (:issue:`55415`) - diff --git a/pandas/_libs/hashtable_class_helper.pxi.in b/pandas/_libs/hashtable_class_helper.pxi.in index 1cf5d734705af..79a0dfc6d723d 100644 --- a/pandas/_libs/hashtable_class_helper.pxi.in +++ b/pandas/_libs/hashtable_class_helper.pxi.in @@ -1128,6 +1128,7 @@ cdef class StringHashTable(HashTable): use_na_value = na_value is not None # assign pointers and pre-filter out missing (if ignore_na) + keep_rval_refs = [] vecs = malloc(n * sizeof(char *)) for i in range(n): val = values[i] @@ -1144,7 +1145,9 @@ cdef class StringHashTable(HashTable): try: v = get_c_string(val) except UnicodeEncodeError: - v = get_c_string(repr(val)) + rval = repr(val) + keep_rval_refs.append(rval) + v = get_c_string(rval) vecs[i] = v # compute From b4157f069c41b20847c1a4c2595dc304bb3b5ad1 Mon Sep 17 00:00:00 2001 From: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com> Date: Sun, 15 Oct 2023 08:32:16 -0400 Subject: [PATCH 2/3] Added test for #45929 and removed superfluous single_cpu mark 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 <72577720+MichaelTiemannOSC@users.noreply.github.com> --- pandas/tests/base/test_unique.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/pandas/tests/base/test_unique.py b/pandas/tests/base/test_unique.py index 4c845d8f24d01..fc9d6fedf20f0 100644 --- a/pandas/tests/base/test_unique.py +++ b/pandas/tests/base/test_unique.py @@ -97,7 +97,6 @@ def test_nunique_null(null_obj, index_or_series_obj): assert obj.nunique(dropna=False) == max(0, num_unique_values) -@pytest.mark.single_cpu def test_unique_bad_unicode(index_or_series): # regression test for #34550 uval = "\ud83d" # smiley emoji @@ -113,6 +112,24 @@ def test_unique_bad_unicode(index_or_series): tm.assert_numpy_array_equal(result, expected) +def test_unique_45929(index_or_series): + # regression test for #45929 + data_list = [ + "1 \udcd6a NY", + "2 \udcd6b NY", + "3 \ud800c NY", + "4 \udcd6d NY", + "5 \udcc3e NY", + ] + + obj = index_or_series(data_list) + assert len(obj.unique()) == len(data_list) + assert len(obj.value_counts()) == len(data_list) + assert len(np.unique(data_list)) == len(data_list) + assert len(set(data_list)) == len(data_list) + assert obj.is_unique + + @pytest.mark.parametrize("dropna", [True, False]) def test_nunique_dropna(dropna): # GH37566 From f33cdc073fe9fd744f95f74a6d815b264ce4384d Mon Sep 17 00:00:00 2001 From: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com> Date: Thu, 16 Nov 2023 15:11:16 +1300 Subject: [PATCH 3/3] Update v2.2.0.rst Correctly alphabetize items in `Other` list. Signed-off-by: Michael Tiemann <72577720+MichaelTiemannOSC@users.noreply.github.com> --- doc/source/whatsnew/v2.2.0.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v2.2.0.rst b/doc/source/whatsnew/v2.2.0.rst index 10d58fc814835..8127ce1fa6eda 100644 --- a/doc/source/whatsnew/v2.2.0.rst +++ b/doc/source/whatsnew/v2.2.0.rst @@ -472,8 +472,8 @@ Other - Bug in :func:`cut` incorrectly allowing cutting of timezone-aware datetimes with timezone-naive bins (:issue:`54964`) - Bug in :func:`infer_freq` and :meth:`DatetimeIndex.inferred_freq` with weekly frequencies and non-nanosecond resolutions (:issue:`55609`) - Bug in :meth:`DataFrame.apply` where passing ``raw=True`` ignored ``args`` passed to the applied function (:issue:`55009`) -- Bug in Cython :meth:`StringHashTable._unique` used ephemeral repr values when UnicodeEncodeError was raised (:issue:`45929`) - Bug in :meth:`Dataframe.from_dict` which would always sort the rows of the created :class:`DataFrame`. (:issue:`55683`) +- Bug in Cython :meth:`StringHashTable._unique` used ephemeral repr values when UnicodeEncodeError was raised (:issue:`45929`) - Bug in rendering ``inf`` values inside a a :class:`DataFrame` with the ``use_inf_as_na`` option enabled (:issue:`55483`) - Bug in rendering a :class:`Series` with a :class:`MultiIndex` when one of the index level's names is 0 not having that name displayed (:issue:`55415`)