Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BUG: read_sql tries to convert blob/varbinary to string with pyarrow backend #60105

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.3.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ MultiIndex
I/O
^^^
- :meth:`DataFrame.to_excel` was storing decimals as strings instead of numbers (:issue:`49598`)
- Bug in :func:`read_sql` causing an unintended exception when byte data was being converted to string when using the pyarrow dtype_backend (:issue:`59242`)
-

Period
Expand Down
4 changes: 0 additions & 4 deletions pandas/core/internals/construction.py
Original file line number Diff line number Diff line change
Expand Up @@ -968,10 +968,6 @@ def convert(arr):
# i.e. maybe_convert_objects didn't convert
convert_to_nullable_dtype = dtype_backend != "numpy"
arr = maybe_infer_to_datetimelike(arr, convert_to_nullable_dtype)
if convert_to_nullable_dtype and arr.dtype == np.dtype("O"):
new_dtype = StringDtype()
arr_cls = new_dtype.construct_array_type()
arr = arr_cls._from_sequence(arr, dtype=new_dtype)
elif dtype_backend != "numpy" and isinstance(arr, np.ndarray):
if arr.dtype.kind in "iufb":
arr = pd_array(arr, copy=False)
Expand Down
24 changes: 24 additions & 0 deletions pandas/tests/io/test_sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -4358,3 +4358,27 @@ def test_xsqlite_if_exists(sqlite_buildin):
(5, "E"),
]
drop_table(table_name, sqlite_buildin)


@pytest.mark.parametrize("dtype_backend", ["pyarrow", "numpy_nullable", lib.no_default])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also found that this was failing for dtype_backend=numpy_nullable so implemented a fix and added test cases for each possible dtype_backend

def test_bytes_column(sqlite_buildin, dtype_backend):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_bytes_column(sqlite_buildin, dtype_backend):
def test_bytes_column(all_connectable, dtype_backend):

Should test this against all databases

pa = pytest.importorskip("pyarrow")
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is well intentioned but can you remove the docstring? We don't use them in tests.

Instead, you can just add a comment pointing to the github issue number in the function body

Regression test for (#59242)
Bytes being returned in a column that could not be converted
to a string would raise a UnicodeDecodeError
when using dtype_backend='pyarrow' or dtype_backend='numpy_nullable'
"""
query = """
select cast(x'0123456789abcdef0123456789abcdef' as blob) a
"""
df = pd.read_sql(query, sqlite_buildin, dtype_backend=dtype_backend)
expected = DataFrame(
[
{
"a": b"\x01#Eg\x89\xab\xcd\xef\x01#Eg\x89\xab\xcd\xef",
}
],
dtype=(pd.ArrowDtype(pa.binary()) if dtype_backend == "pyarrow" else "O"),
)
tm.assert_frame_equal(df, expected)
Loading