Repository: pandas-dev/pandas
Pull request: API: Disallow NaN in StringArray constructor
Closes https://github.com/pandas-dev/pandas/issues/30966

~There were a few ways we could have done this. I opted for this way since it still only requires a single pass over the data to validate the values. Other ways like checking for `np.isnan` after checking `is_string_array` would have required a second pass.~

I changed the implementation in subsequent commits. The basic idea is the same, don't allow NaN in the array passed to StringArray, so that we only make a single pass over the data. We do this by changing `StringValidator.is_valid_null` to only allow NA. This required a small change to `PandasArray._from_sequence`, since previously we relied on creating a temporarily invalid StringArray before doing an inplace `__setitem__` to replace NaNs with NA.

cc @tsvikas.


Problem statement:
str accessor functions returns float(NaN) instead of pd.NA
#### Code Sample

```python
import pandas as pd
s = pd.Series(['A', 'B', 'C', 'Aaba', 'Baca', np.nan, 'CABA', 'dog', 'cat'], dtype="string")
type(s.str.lower()[5])  # returns <class 'float'>
```
#### Problem description
the `str` accessor, when working on string-typed series, should return a string-typed series, which should be an array of [`string`, `pd.NA`] only, but it seems that some functions (see list below) can return series that contains `float('nan')`.

#### Affected functions
As of now, I found these str accessor functions to be affected:
`upper` `lower` `replace`
also, `extract(expand=False)` on a string type series returns an object type series, which seems unintended as well.

#### Expected Output
`<class 'pandas._libs.missing.NAType'>`
#### Output of ``pd.show_versions()``

<details>

```
INSTALLED VERSIONS
------------------
commit           : None
python           : 3.8.0.final.0
python-bits      : 64
OS               : Linux
OS-release       : 5.0.0-38-generic
machine          : x86_64
processor        : x86_64
byteorder        : little
LC_ALL           : None
LANG             : en_IL
LOCALE           : en_IL.UTF-8

pandas           : 1.0.0rc0
numpy            : 1.18.1
pytz             : 2019.3
dateutil         : 2.8.1
pip              : 18.1
setuptools       : 40.8.0
Cython           : None
pytest           : None
hypothesis       : None
sphinx           : None
blosc            : None
feather          : None
xlsxwriter       : None
lxml.etree       : None
html5lib         : None
pymysql          : None
psycopg2         : None
jinja2           : None
IPython          : None
pandas_datareader: None
bs4              : None
bottleneck       : None
fastparquet      : None
gcsfs            : None
lxml.etree       : None
matplotlib       : None
numexpr          : None
odfpy            : None
openpyxl         : None
pandas_gbq       : None
pyarrow          : None
pytables         : None
pytest           : None
s3fs             : None
scipy            : None
sqlalchemy       : None
tables           : None
tabulate         : None
xarray           : None
xlrd             : None
xlwt             : None
xlsxwriter       : None
numba            : None
```
</details>

Hints from issue discussion:
Thanks for the report. Seems to be with the StringArray constructor when passed an ndarray,

```python
In [2]: import pandas as pd

In [3]: import numpy as np

In [4]: values = np.array(['a', np.nan], dtype=object)

In [6]: pd.arrays.StringArray(values)
Out[6]:
<StringArray>
['a', nan]
Length: 2, dtype: string
```
I don't recall if we expect / document that the values passed to StringArray should be strings / NA (not NaN). When we go through `pd.array`, things work correctly since `StringArray._from_sequence` does the NaN -> NA substitution. My preference is for

- `StringArray(np.array(['a', np.nan], dtype=object))` to raise, since the user has passed NaN insted of NA
- update `core/strings.py` to use NA in the correct places or go through `_from_sequence`.
Thanks for the reply! Any way to help to solve the issue?

On a similar issue, `str.extract(expand=False)` on a string typed series returns an object typed dataframe, which seems to be unintended as well. (to compare, `str.extract(expand=True)` on a string typed series returns an string typed dataframe)
To have `StringArray(np.array(['a', np.nan], dtype=object))`, we'll need to update `_libs.lib.is_string_array`. Other places use it, so we'll need an additional flag like `na_only` to exclude NaN from the NA check.
The `expand=False` does look buggy. I thought there were cases where we returned a tuple of matches, but apparenlty not. Can you open an issue just for that?

Commit message: Merge remote-tracking branch 'upstream/master' into 30966-str-validate

Patch to review:
diff --git a/pandas/_libs/lib.pyx b/pandas/_libs/lib.pyx
index 719db5c03f07f..acd74591134bc 100644
--- a/pandas/_libs/lib.pyx
+++ b/pandas/_libs/lib.pyx
@@ -1624,6 +1624,10 @@ cdef class StringValidator(Validator):
     cdef inline bint is_array_typed(self) except -1:
         return issubclass(self.dtype.type, np.str_)
 
+    cdef bint is_valid_null(self, object value) except -1:
+        # We deliberately exclude None / NaN here since StringArray uses NA
+        return value is C_NA
+
 
 cpdef bint is_string_array(ndarray values, bint skipna=False):
     cdef:
diff --git a/pandas/core/arrays/numpy_.py b/pandas/core/arrays/numpy_.py
index 4db3d3010adaf..6476614a19ac9 100644
--- a/pandas/core/arrays/numpy_.py
+++ b/pandas/core/arrays/numpy_.py
@@ -162,6 +162,8 @@ def _from_sequence(cls, scalars, dtype=None, copy=False):
         result = np.asarray(scalars, dtype=dtype)
         if copy and result is scalars:
             result = result.copy()
+        # _coerce_from_sequence_values is useful for StringArray
+        result = cls._coerce_from_sequence_values(result, copy=copy)
         return cls(result)
 
     @classmethod
diff --git a/pandas/core/arrays/string_.py b/pandas/core/arrays/string_.py
index 84130132de4dc..c485d1f50dc9d 100644
--- a/pandas/core/arrays/string_.py
+++ b/pandas/core/arrays/string_.py
@@ -93,9 +93,6 @@ class StringArray(PandasArray):
        StringArray is considered experimental. The implementation and
        parts of the API may change without warning.
 
-       In particular, the NA value used may change to no longer be
-       ``numpy.nan``.
-
     Parameters
     ----------
     values : array-like
@@ -104,8 +101,11 @@ class StringArray(PandasArray):
         .. warning::
 
            Currently, this expects an object-dtype ndarray
-           where the elements are Python strings. This may
-           change without warning in the future.
+           where the elements are Python strings or :attr:`pandas.NA`.
+           This may change without warning in the future. Use
+           :meth:`pandas.array` with ``dtype="string"`` for a stable way of
+           creating a `StringArray` from any sequence.
+
     copy : bool, default False
         Whether to copy the array of data.
 
@@ -119,6 +119,8 @@ class StringArray(PandasArray):
 
     See Also
     --------
+    array
+        The recommended function for creating a StringArray.
     Series.str
         The string methods are available on Series backed by
         a StringArray.
@@ -165,12 +167,10 @@ def __init__(self, values, copy=False):
     def _validate(self):
         """Validate that we only store NA or strings."""
         if len(self._ndarray) and not lib.is_string_array(self._ndarray, skipna=True):
-            raise ValueError(
-                "StringArray requires a sequence of strings or missing values."
-            )
+            raise ValueError("StringArray requires a sequence of strings or pandas.NA")
         if self._ndarray.dtype != "object":
             raise ValueError(
-                "StringArray requires a sequence of strings. Got "
+                "StringArray requires a sequence of strings or pandas.NA. Got "
                 f"'{self._ndarray.dtype}' dtype instead."
             )
 
@@ -178,12 +178,22 @@ def _validate(self):
     def _from_sequence(cls, scalars, dtype=None, copy=False):
         if dtype:
             assert dtype == "string"
-        result = super()._from_sequence(scalars, dtype=object, copy=copy)
+
+        result = np.asarray(scalars, dtype="object")
+        if copy and result is scalars:
+            result = result.copy()
+
         # Standardize all missing-like values to NA
         # TODO: it would be nice to do this in _validate / lib.is_string_array
         # We are already doing a scan over the values there.
-        result[result.isna()] = StringDtype.na_value
-        return result
+        na_values = isna(result)
+        if na_values.any():
+            if result is scalars:
+                # force a copy now, if we haven't already
+                result = result.copy()
+            result[na_values] = StringDtype.na_value
+
+        return cls(result)
 
     @classmethod
     def _from_sequence_of_strings(cls, strings, dtype=None, copy=False):
diff --git a/pandas/core/strings.py b/pandas/core/strings.py
index f8d9eeb211a1e..0323eafff8dee 100644
--- a/pandas/core/strings.py
+++ b/pandas/core/strings.py
@@ -8,6 +8,7 @@
 import numpy as np
 
 import pandas._libs.lib as lib
+import pandas._libs.missing as libmissing
 import pandas._libs.ops as libops
 from pandas._typing import ArrayLike, Dtype
 from pandas.util._decorators import Appender
@@ -118,12 +119,15 @@ def cat_safe(list_of_columns: List, sep: str):
     return result
 
 
-def _na_map(f, arr, na_result=np.nan, dtype=object):
-    # should really _check_ for NA
+def _na_map(f, arr, na_result=None, dtype=object):
     if is_extension_array_dtype(arr.dtype):
+        if na_result is None:
+            na_result = libmissing.NA
         # just StringDtype
         arr = extract_array(arr)
         return _map_stringarray(f, arr, na_value=na_result, dtype=dtype)
+    if na_result is None:
+        na_result = np.nan
     return _map_object(f, arr, na_mask=True, na_value=na_result, dtype=dtype)
 
 
diff --git a/pandas/tests/arrays/string_/test_string.py b/pandas/tests/arrays/string_/test_string.py
index 33e68f029922e..8e17d3c8cd1e0 100644
--- a/pandas/tests/arrays/string_/test_string.py
+++ b/pandas/tests/arrays/string_/test_string.py
@@ -194,6 +194,25 @@ def test_constructor_raises():
     with pytest.raises(ValueError, match="sequence of strings"):
         pd.arrays.StringArray(np.array([]))
 
+    with pytest.raises(ValueError, match="strings or pandas.NA"):
+        pd.arrays.StringArray(np.array(["a", np.nan], dtype=object))
+
+    with pytest.raises(ValueError, match="strings or pandas.NA"):
+        pd.arrays.StringArray(np.array(["a", None], dtype=object))
+
+    with pytest.raises(ValueError, match="strings or pandas.NA"):
+        pd.arrays.StringArray(np.array(["a", pd.NaT], dtype=object))
+
+
+@pytest.mark.parametrize("copy", [True, False])
+def test_from_sequnce_no_mutate(copy):
+    a = np.array(["a", np.nan], dtype=object)
+    original = a.copy()
+    result = pd.arrays.StringArray._from_sequence(a, copy=copy)
+    expected = pd.arrays.StringArray(np.array(["a", pd.NA], dtype=object))
+    tm.assert_extension_array_equal(result, expected)
+    tm.assert_numpy_array_equal(a, original)
+
 
 @pytest.mark.parametrize("skipna", [True, False])
 @pytest.mark.xfail(reason="Not implemented StringArray.sum")
diff --git a/pandas/tests/dtypes/test_inference.py b/pandas/tests/dtypes/test_inference.py
index d022b0e97877a..5eb85de2b90f5 100644
--- a/pandas/tests/dtypes/test_inference.py
+++ b/pandas/tests/dtypes/test_inference.py
@@ -1114,11 +1114,16 @@ def test_is_string_array(self):
 
         assert lib.is_string_array(np.array(["foo", "bar"]))
         assert not lib.is_string_array(
-            np.array(["foo", "bar", np.nan], dtype=object), skipna=False
+            np.array(["foo", "bar", pd.NA], dtype=object), skipna=False
         )
         assert lib.is_string_array(
+            np.array(["foo", "bar", pd.NA], dtype=object), skipna=True
+        )
+        # NaN is not valid for string array, just NA
+        assert not lib.is_string_array(
             np.array(["foo", "bar", np.nan], dtype=object), skipna=True
         )
+
         assert not lib.is_string_array(np.array([1, 2]))
 
     def test_to_object_array_tuples(self):
diff --git a/pandas/tests/test_strings.py b/pandas/tests/test_strings.py
index a92f917820bd0..c37c78f3b9235 100644
--- a/pandas/tests/test_strings.py
+++ b/pandas/tests/test_strings.py
@@ -3521,7 +3521,7 @@ def test_string_array(any_string_method):
 
     if isinstance(expected, Series):
         if expected.dtype == "object" and lib.is_string_array(
-            expected.values, skipna=True
+            expected.dropna().values,
         ):
             assert result.dtype == "string"
             result = result.astype(object)


Code review comments:
diff_hunk: @@ -162,6 +162,8 @@ def _from_sequence(cls, scalars, dtype=None, copy=False):
         result = np.asarray(scalars, dtype=dtype)
         if copy and result is scalars:
             result = result.copy()
+        # _coerce_from_sequence_values is useful for StringArray
+        result = cls._coerce_from_sequence_values(result, copy=copy)
path: pandas/core/arrays/numpy_.py
text: @user1:
where is this?

@author:
Sorry, leftover change that I forgot to remove.

diff_hunk: @@ -194,6 +194,25 @@ def test_constructor_raises():
     with pytest.raises(ValueError, match="sequence of strings"):
         pd.arrays.StringArray(np.array([]))
 
+    with pytest.raises(ValueError, match="strings or pandas.NA"):
+        pd.arrays.StringArray(np.array(["a", np.nan], dtype=object))
+
+    with pytest.raises(ValueError, match="strings or pandas.NA"):
+        pd.arrays.StringArray(np.array(["a", None], dtype=object))
+
+    with pytest.raises(ValueError, match="strings or pandas.NA"):
+        pd.arrays.StringArray(np.array(["a", pd.NaT], dtype=object))
+
+
+@pytest.mark.parametrize("copy", [True, False])
+def test_from_sequnce_no_mutate(copy):
path: pandas/tests/arrays/string_/test_string.py
text: @user1:
sp on sequence