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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2244869
Add fix for #59242
kastkeepitjumpinlikekangaroos Oct 25, 2024
bd00fc5
add skip import
kastkeepitjumpinlikekangaroos Oct 25, 2024
6a23f05
address comment
kastkeepitjumpinlikekangaroos Nov 12, 2024
2f261c8
merged main
kastkeepitjumpinlikekangaroos Nov 12, 2024
8f900b8
also fix for dtype_backend=numpy_nullable
kastkeepitjumpinlikekangaroos Nov 13, 2024
536b1ed
Merge branch 'main' into fix-59242
kastkeepitjumpinlikekangaroos Nov 14, 2024
8965a1d
merge main
kastkeepitjumpinlikekangaroos Nov 17, 2024
a32b4a6
fix
kastkeepitjumpinlikekangaroos Nov 17, 2024
f412c72
Merge branch 'main' into fix-59242
kastkeepitjumpinlikekangaroos Nov 20, 2024
a0200d0
address comment
kastkeepitjumpinlikekangaroos Nov 20, 2024
10ca030
Merge branch 'main' into fix-59242
kastkeepitjumpinlikekangaroos Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.3.0.rst
View file
Open in desktop
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
34 changes: 30 additions & 4 deletions pandas/core/internals/construction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from pandas._config import using_string_dtype

from pandas._libs import lib
from pandas.compat._optional import import_optional_dependency

from pandas.core.dtypes.astype import astype_is_view
from pandas.core.dtypes.cast import (
Expand All @@ -34,7 +35,10 @@
is_object_dtype,
is_scalar,
)
from pandas.core.dtypes.dtypes import ExtensionDtype
from pandas.core.dtypes.dtypes import (
ArrowDtype,
ExtensionDtype,
)
from pandas.core.dtypes.generic import (
ABCDataFrame,
ABCSeries,
Expand Down Expand Up @@ -968,9 +972,31 @@ def convert(arr):
# i.e. maybe_convert_objects didn't convert
arr = maybe_infer_to_datetimelike(arr)
if dtype_backend != "numpy" and arr.dtype == np.dtype("O"):
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the wrong place to be doing this; in the sql.py module can't we read in the type of the database and only try to convert BINARY types to Arrow binary types?

Copy link
Author

Choose a reason for hiding this comment

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

Based on some local testing using the ADBC driver I can confirm that it yields a pandas.core.arrays.arrow.array.ArrowExtensionArray with ._dtype of pandas.ArrowDtype. When the query returns a bytes type column we get a .type of bytes, and likewise a .type of string is returned for a string type column. Seems like we don't need to do any conversions when using the ADBC driver as you've stated if I'm understanding correctly here!

Wondering if it makes sense to remove the code here trying to convert based on a dtype_backend != 'numpy' since this will fix the cause of the exception in the issue? and maybe raise an exception when trying to use a pyarrow dtype_backend with the SQLiteDatabase connection type here: https://github.com/pandas-dev/pandas/blob/main/pandas/io/sql.py#L695 ?

Copy link
Member

Choose a reason for hiding this comment

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

I think the general problem is that pandas does not have a first class "binary" data type, so I'm not sure how to solve this for anything but the pyarrow backend.

With the pyarrow backend, I think you can still move this logic to sql.py and check the type of the column coming back from the database. If it is a binary type in the database, using the PyArrow binary type with that backend makes sense.

Not sure if @mroeschke has other thoughts to the general issue. This is likely another good use case to track in PDEP-13 #58455

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this is the incorrect place to handle this conversion logic and this should only be a valid conversion for the pyarrow backend (ArrowExtensionArray._from_sequence should be able to return a binary type with binary data.)

Copy link
Author

Choose a reason for hiding this comment

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

in sql.py it looks like the result of this conversion is being overwritten when the dtype_backend is pyarrow: https://github.com/pandas-dev/pandas/blob/main/pandas/io/sql.py#L181 and the dtype returned by the current logic is ArrowDtype(pa.binary()) for the example in the issue, so maybe just removing the conversion logic is all that's needed to resolve this issue? I've removed the block doing the conversion and added a test case showing that the resulting df has a dtype of ArrowDtype(pa.binary()) when the dtype_backend='pyarrow'

new_dtype = StringDtype()
arr_cls = new_dtype.construct_array_type()
arr = arr_cls._from_sequence(arr, dtype=new_dtype)
# Addressing (#59242)
# Byte data that could not be decoded into
# a string would throw a UnicodeDecodeError exception

# Try and greedily convert to string
if dtype_backend == "pyarrow":
pa = import_optional_dependency("pyarrow")
try:
str_dtype = ArrowDtype(pa.string())
str_cls = str_dtype.construct_array_type()
arr = str_cls._from_sequence(arr, dtype=str_dtype)
except pa.lib.ArrowInvalid:
# in this case convert to pyarrow binary
bin_dtype = ArrowDtype(pa.binary())
bin_cls = bin_dtype.construct_array_type()
arr = bin_cls._from_sequence(arr, dtype=bin_dtype)
else:
try:
new_dtype = StringDtype()
arr_cls = new_dtype.construct_array_type()
arr = arr_cls._from_sequence(arr, dtype=new_dtype)
except UnicodeDecodeError:
# in this case do nothing
pass

elif dtype_backend != "numpy" and isinstance(arr, np.ndarray):
if arr.dtype.kind in "iufb":
arr = pd_array(arr, copy=False)
Expand Down
16 changes: 16 additions & 0 deletions pandas/tests/io/test_sql.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4355,3 +4355,19 @@ 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])
Copy link
Author

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

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)
assert df.a.values[0] == b"\x01#Eg\x89\xab\xcd\xef\x01#Eg\x89\xab\xcd\xef"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use our built-in test helpers instead? I think you can just do:

result = pd.read_sql(...)
expected = pd.DataFrame({"a": ...}, dtype=pd.ArrowDtype(pa.binary()))
tm.assert_frame_equal(result, expected)

What data type does this produce currently with the numpy_nullable backend - object?

Copy link
Author

Choose a reason for hiding this comment

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

for sure, changed the testing logic over to using this! for numpy_nullable and lib.no_default the dtype returned is an object