BUG: length-aware string hashtable keys, fixing embedded-NUL collapse in unique/factorize/groupby (#66512)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: jbrockmendel

doc/source/whatsnew/v3.1.0.rst

@@ -427,6 +427,7 @@ Strings
 - Bug in :meth:`Series.str.rsplit` and :meth:`Index.str.rsplit` silently accepting a compiled regex and returning incorrect results (:issue:`29633`)
 - Bug in :meth:`Series.str.split` with :class:`ArrowDtype` ``string`` not inferring regex for multi-character patterns when ``regex=None``, causing the pattern to be treated as a literal instead of a regular expression (:issue:`58321`)
 - Bug in :meth:`Series.unique`, :meth:`Index.unique` and :func:`factorize` on object dtype returning incorrect results when the values were not UTF-8 encodable, e.g. lone surrogates (:issue:`34550`)
+- Bug in :meth:`Series.unique`, :meth:`Index.unique`, :meth:`Series.nunique`, :func:`factorize` and ``groupby`` on object dtype, and on ``str``/``string`` dtype backed by python storage, collapsing distinct strings that are identical up to an embedded NUL byte, e.g. ``""`` and ``"\x00"``, into a single value (:issue:`34551`)
 
 Interval
 ^^^^^^^^
@@ -511,8 +512,10 @@ I/O
 - Fixed bug in :func:`read_csv` with ``engine="pyarrow"`` where a ``defaultdict`` passed as ``dtype`` did not apply its default to columns not explicitly listed (:issue:`41574`)
 - Fixed bug in :func:`read_csv` with ``engine="pyarrow"`` where an empty ``usecols`` (e.g. ``usecols=[]``) was ignored and returned all columns instead of an empty frame, unlike the other engines (:issue:`66056`)
 - Fixed bug in :func:`read_csv` with ``engine="pyarrow"`` where passing tuples in ``names`` produced flat columns instead of :class:`MultiIndex` columns as with the other engines (:issue:`65862`)
+- Fixed bug in :func:`read_csv` with the ``c`` engine and ``dtype="category"`` where ``encoding_errors`` was ignored, so an undecodable byte raised ``UnicodeDecodeError`` even with ``encoding_errors="replace"`` (:issue:`66525`)
 - Fixed bug in :func:`read_csv` with the ``c`` engine where a quoted field containing an embedded NUL byte was silently truncated at the NUL under the default string dtype or ``dtype_backend="pyarrow"`` (:issue:`66415`)
 - Fixed bug in :func:`read_csv` with the ``c`` engine where an embedded ``\r`` followed by a space in an unquoted field could cause an infinite re-parsing loop, producing spurious rows or a buffer overflow (:issue:`51141`)
+- Fixed bug in :func:`read_csv` with the ``c`` engine where two fields differing only after an embedded NUL byte were read as the same value with an explicit string-like ``dtype`` (``object``, ``"str"``, ``"string"`` or ``"category"``) (:issue:`66525`)
 - Fixed bug in :func:`read_excel` where usage of ``skiprows`` could lead to an infinite loop (:issue:`64027`)
 - Fixed bug in :func:`read_excel` with the ``openpyxl`` engine where reading a sheet set its ``max_row`` and ``max_column`` to ``None`` on the workbook exposed through ``ExcelFile.book`` (:issue:`63010`)
 - Fixed bug in :func:`read_sas` where ``encoding="infer"`` raised ``LookupError: unknown encoding: infer`` instead of falling back to latin-1, for SAS7BDAT files recording an encoding pandas does not recognize and for XPORT files, which record no encoding at all (:issue:`66470`)

pandas/_libs/hashtable.pxd

@@ -22,6 +22,7 @@ from pandas._libs.khash cimport (
     kh_int64_t,
     kh_pymap_t,
     kh_str_t,
+    kh_strview_t,
     kh_uint8_t,
     kh_uint16_t,
     kh_uint32_t,

pandas/_libs/hashtable.pyx

@@ -29,6 +29,7 @@ from pandas._libs.khash cimport (
     kh_needed_n_buckets,
     kh_python_hash_equal,
     kh_python_hash_func,
+    kh_strview,
     khiter_t,
 )
 from pandas._libs.missing cimport (

pandas/_libs/hashtable_class_helper.pxi.in

@@ -3,7 +3,7 @@ Template for each `dtype` helper function for hashtable
 
 WARNING: DO NOT edit .pxi FILE directly, .pxi is generated from .pxi.in
 """
-from cpython.unicode cimport PyUnicode_AsUTF8
+from cpython.unicode cimport PyUnicode_AsUTF8AndSize
 
 {{py:
 
@@ -964,6 +964,11 @@ cdef class {{name}}Factorizer(Factorizer):
 
 
 cdef class StringHashTable(HashTable):
+    # Keys borrow each str's cached UTF-8 buffer via PyUnicode_AsUTF8AndSize,
+    # which yields the length alongside the pointer, so keying on an explicit
+    # length costs no extra pass. The buffer lives exactly as long as the str
+    # does, so callers must keep the str alive while the key is in the table.
+
     # these by-definition *must* be strings
     # or a sentinel np.nan / None missing value
     na_string_sentinel = '__nan__'
@@ -984,8 +989,8 @@ cdef class StringHashTable(HashTable):
     def sizeof(self, deep: bool = False) -> int:
         overhead = 4 * sizeof(uint32_t) + 3 * sizeof(uint32_t*)
         for_flags = max(1, self.table.n_buckets >> 5) * sizeof(uint32_t)
-        for_pairs =  self.table.n_buckets * (sizeof(char *) +      # keys
-                                             sizeof(Py_ssize_t))   # vals
+        for_pairs =  self.table.n_buckets * (sizeof(kh_strview_t) +  # keys
+                                             sizeof(Py_ssize_t))     # vals
         return overhead + for_flags + for_pairs
 
     def get_state(self) -> dict[str, int]:
@@ -1000,8 +1005,11 @@ cdef class StringHashTable(HashTable):
     cpdef get_item(self, str val):
         cdef:
             khiter_t k
-            const char *v
-        v = PyUnicode_AsUTF8(val)
+            kh_strview_t v
+            Py_ssize_t length
+            const char *ptr
+        ptr = PyUnicode_AsUTF8AndSize(val, &length)
+        v = kh_strview(ptr, length)
 
         k = kh_get_str(self.table, v)
         if k != self.table.n_buckets:
@@ -1013,9 +1021,12 @@ cdef class StringHashTable(HashTable):
         cdef:
             khiter_t k
             int ret = 0
-            const char *v
+            kh_strview_t v
+            Py_ssize_t length
+            const char *ptr
 
-        v = PyUnicode_AsUTF8(key)
+        ptr = PyUnicode_AsUTF8AndSize(key, &length)
+        v = kh_strview(ptr, length)
 
         k = kh_put_str(self.table, v, &ret)
         if kh_exist_str(self.table, k):
@@ -1033,16 +1044,17 @@ cdef class StringHashTable(HashTable):
             intp_t *resbuf = <intp_t*>labels.data
             khiter_t k
             kh_str_t *table = self.table
-            const char *v
-            const char **vecs
+            kh_strview_t *vecs
+            Py_ssize_t length
+            const char *ptr
 
-        vecs = <const char **>malloc(n * sizeof(char *))
+        vecs = <kh_strview_t *>malloc(n * sizeof(kh_strview_t))
         if vecs is NULL:
             raise MemoryError()
         for i in range(n):
             val = values[i]
-            v = PyUnicode_AsUTF8(val)
-            vecs[i] = v
+            ptr = PyUnicode_AsUTF8AndSize(val, &length)
+            vecs[i] = kh_strview(ptr, length)
 
         with nogil:
             for i in range(n):
@@ -1064,24 +1076,27 @@ cdef class StringHashTable(HashTable):
             Py_ssize_t i, n = len(values)
             int ret = 0
             object val
-            const char *v
+            kh_strview_t v
+            kh_strview_t *vecs
+            Py_ssize_t length
+            const char *ptr
             khiter_t k
             intp_t[::1] locs = np.empty(n, dtype=np.intp)
 
         # these by-definition *must* be strings
-        vecs = <const char **>malloc(n * sizeof(char *))
+        vecs = <kh_strview_t *>malloc(n * sizeof(kh_strview_t))
         if vecs is NULL:
             raise MemoryError()
         for i in range(n):
             val = values[i]
 
             if isinstance(val, str):
-                # GH#31499 if we have an np.str_ PyUnicode_AsUTF8 won't recognize
-                #  it as a str, even though isinstance does.
-                v = PyUnicode_AsUTF8(<str>val)
+                # GH#31499 if we have an np.str_ PyUnicode_AsUTF8AndSize won't
+                #  recognize it as a str, even though isinstance does.
+                ptr = PyUnicode_AsUTF8AndSize(<str>val, &length)
             else:
-                v = PyUnicode_AsUTF8(self.na_string_sentinel)
-            vecs[i] = v
+                ptr = PyUnicode_AsUTF8AndSize(self.na_string_sentinel, &length)
+            vecs[i] = kh_strview(ptr, length)
 
         with nogil:
             for i in range(n):
@@ -1103,24 +1118,26 @@ cdef class StringHashTable(HashTable):
             Py_ssize_t i, n = len(values)
             int ret = 0
             object val
-            const char *v
-            const char **vecs
+            kh_strview_t v
+            kh_strview_t *vecs
+            Py_ssize_t length
+            const char *ptr
             khiter_t k
 
         # these by-definition *must* be strings
-        vecs = <const char **>malloc(n * sizeof(char *))
+        vecs = <kh_strview_t *>malloc(n * sizeof(kh_strview_t))
         if vecs is NULL:
             raise MemoryError()
         for i in range(n):
             val = values[i]
 
             if isinstance(val, str):
-                # GH#31499 if we have an np.str_ PyUnicode_AsUTF8 won't recognize
-                #  it as a str, even though isinstance does.
-                v = PyUnicode_AsUTF8(<str>val)
+                # GH#31499 if we have an np.str_ PyUnicode_AsUTF8AndSize won't
+                #  recognize it as a str, even though isinstance does.
+                ptr = PyUnicode_AsUTF8AndSize(<str>val, &length)
             else:
-                v = PyUnicode_AsUTF8(self.na_string_sentinel)
-            vecs[i] = v
+                ptr = PyUnicode_AsUTF8AndSize(self.na_string_sentinel, &length)
+            vecs[i] = kh_strview(ptr, length)
 
         with nogil:
             for i in range(n):
@@ -1174,8 +1191,10 @@ cdef class StringHashTable(HashTable):
             int64_t[::1] uindexer
             int ret = 0
             object val
-            const char *v
-            const char **vecs
+            kh_strview_t v
+            kh_strview_t *vecs
+            Py_ssize_t length
+            const char *ptr
             khiter_t k
             bint use_na_value
             bint non_null_na_value
@@ -1188,7 +1207,7 @@ cdef class StringHashTable(HashTable):
         non_null_na_value = not checknull(na_value)
 
         # assign pointers and pre-filter out missing (if ignore_na)
-        vecs = <const char **>malloc(n * sizeof(char *))
+        vecs = <kh_strview_t *>malloc(n * sizeof(kh_strview_t))
         if vecs is NULL:
             raise MemoryError()
         for i in range(n):
@@ -1209,15 +1228,15 @@ cdef class StringHashTable(HashTable):
             else:
                 # if ignore_na is False, we also stringify NaN/None/etc.
                 try:
-                    v = PyUnicode_AsUTF8(<str>val)
+                    ptr = PyUnicode_AsUTF8AndSize(<str>val, &length)
                 except UnicodeEncodeError:
                     # GH#34550 val is not utf-8 encodable (e.g. a lone
                     #  surrogate). Retain the repr in `refs` so the buffer
-                    #  `v` points to outlives the nogil loop below.
+                    #  `ptr` points to outlives the nogil loop below.
                     val = repr(val)
                     refs.append(val)
-                    v = PyUnicode_AsUTF8(<str>val)
-                vecs[i] = v
+                    ptr = PyUnicode_AsUTF8AndSize(<str>val, &length)
+                vecs[i] = kh_strview(ptr, length)
 
         # compute
         with nogil:

pandas/_libs/include/pandas/vendored/klib/khash.h

@@ -749,7 +749,10 @@ typedef const char *kh_cstr_t;
 #define kh_exist_set_int8(h, k) (kh_exist(h, k))
 #define kh_exist_set_uint8(h, k) (kh_exist(h, k))
 
-KHASH_MAP_INIT_STR(str, size_t)
+// kh_str_t is deliberately *not* instantiated here. pandas keys its string
+// tables on an explicit (pointer, length) pair so that values containing
+// embedded NUL bytes are not truncated; see KHASH_MAP_INIT_STRVIEW in
+// khash_python.h.
 KHASH_MAP_INIT_INT(int32, size_t)
 KHASH_MAP_INIT_UINT(uint32, size_t)
 KHASH_MAP_INIT_INT64(int64, size_t)

pandas/_libs/include/pandas/vendored/klib/khash_python.h

@@ -380,10 +380,75 @@ KHASH_SET_INIT_PYOBJECT(pyset)
 #define kh_exist_pymap(h, k) (kh_exist(h, k))
 #define kh_exist_pyset(h, k) (kh_exist(h, k))
 
-KHASH_MAP_INIT_STR(strbox, kh_pyobject_t)
+// Length-aware string key.
+//
+// A bare `const char *` key (klib's kh_cstr_t) hashes and compares only up to
+// the first NUL byte, so distinct values that share a NUL-terminated prefix
+// collapse into a single entry. Python strings, CSV fields and Arrow string
+// buffers can all contain embedded NULs, which made that silently return wrong
+// results (GH#34551, GH#66525). Keying on an explicit (pointer, length) pair
+// fixes it, and additionally lets a key point at a slice of a larger buffer
+// without copying or NUL-terminating it.
+//
+// The pointed-to bytes are borrowed, exactly as for kh_cstr_t: the caller must
+// keep them alive for as long as the entry is in the table.
+typedef struct {
+  const char *ptr;
+  size_t len;
+} kh_strview_t;
+
+static inline kh_strview_t kh_strview(const char *ptr, size_t len) {
+  kh_strview_t result;
+  result.ptr = ptr;
+  result.len = len;
+  return result;
+}
+
+// X31 over exactly `len` bytes. Bit-identical to __ac_X31_hash_string for
+// NUL-free input, so bucket distribution -- and hence performance -- is
+// unchanged for the data that used to work.
+static inline khuint_t kh_strview_hash_func(kh_strview_t key) {
+  khuint_t h = 0;
+  if (key.len) {
+    h = (khuint_t)key.ptr[0];
+    for (size_t i = 1; i < key.len; ++i)
+      h = (h << 5) - h + (khuint_t)key.ptr[i];
+  }
+  return h;
+}
+
+// The length check alone settles most probes, and the byte loop is deliberate:
+// memcmp with a runtime length does not inline, and one libc call per token
+// costs ~9% on a bool column, where a hashset lookup hits on nearly every
+// token and there is little other per-token work to absorb it.
+static inline int kh_strview_hash_equal(kh_strview_t a, kh_strview_t b) {
+  if (a.len != b.len)
+    return 0;
+  for (size_t i = 0; i < a.len; ++i)
+    if (a.ptr[i] != b.ptr[i])
+      return 0;
+  return 1;
+}
+
+#define KHASH_MAP_INIT_STRVIEW(name, khval_t)                                  \
+  KHASH_INIT(name, kh_strview_t, khval_t, 1, kh_strview_hash_func,             \
+             kh_strview_hash_equal)
+
+KHASH_MAP_INIT_STRVIEW(str, size_t)
+KHASH_MAP_INIT_STRVIEW(strbox, kh_pyobject_t)
+
+// NUL-terminated keys for kh_str_starts_t only. The na/true/false-values
+// tables still hash and compare up to the first NUL -- the pre-existing
+// behavior. Giving them a length-aware key is what fixes GH#19886, and is
+// deliberately left to a follow-up because it changes user-visible NA
+// semantics (and hence dtype inference). Do not "fix" it here by deriving the
+// length with strlen: kh_get_str_starts_item runs per token per column on the
+// read_csv hot path, and that strlen alone measured 4-21% on every csvbench
+// core case.
+KHASH_MAP_INIT_STR(strz, size_t)
 
 typedef struct {
-  kh_str_t *table;
+  kh_strz_t *table;
   int starts[256];
 } kh_str_starts_t;
 
@@ -392,13 +457,13 @@ typedef kh_str_starts_t *p_kh_str_starts_t;
 static inline p_kh_str_starts_t kh_init_str_starts(void) {
   kh_str_starts_t *result =
       (kh_str_starts_t *)KHASH_CALLOC(1, sizeof(kh_str_starts_t));
-  result->table = kh_init_str();
+  result->table = kh_init_strz();
   return result;
 }
 
-static inline khuint_t kh_put_str_starts_item(kh_str_starts_t *table, char *key,
-                                              int *ret) {
-  khuint_t result = kh_put_str(table->table, key, ret);
+static inline khuint_t kh_put_str_starts_item(kh_str_starts_t *table,
+                                              const char *key, int *ret) {
+  khuint_t result = kh_put_strz(table->table, key, ret);
   if (*ret != 0) {
     table->starts[(unsigned char)key[0]] = 1;
   }
@@ -409,19 +474,19 @@ static inline khuint_t kh_get_str_starts_item(const kh_str_starts_t *table,
                                               const char *key) {
   unsigned char ch = *key;
   if (table->starts[ch]) {
-    if (ch == '\0' || kh_get_str(table->table, key) != table->table->n_buckets)
+    if (ch == '\0' || kh_get_strz(table->table, key) != table->table->n_buckets)
       return 1;
   }
   return 0;
 }
 
 static inline void kh_destroy_str_starts(kh_str_starts_t *table) {
-  kh_destroy_str(table->table);
+  kh_destroy_strz(table->table);
   KHASH_FREE(table);
 }
 
 static inline void kh_resize_str_starts(kh_str_starts_t *table, khuint_t val) {
-  kh_resize_str(table->table, val);
+  kh_resize_strz(table->table, val);
 }
 
 // utility function: given the number of elements