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

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

Open
wants to merge 16 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 9 commits
Commits
Show all changes
16 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
ba2e82d
address comment
kastkeepitjumpinlikekangaroos Dec 4, 2024
8dd45ca
Merge branch 'fix-59242' of github.com:kastkeepitjumpinlikekangaroos/…
kastkeepitjumpinlikekangaroos Dec 4, 2024
602194a
remove cast
kastkeepitjumpinlikekangaroos Dec 4, 2024
f8ae285
fix test
kastkeepitjumpinlikekangaroos Dec 4, 2024
02514e0
fix logic
kastkeepitjumpinlikekangaroos Dec 4, 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
4 changes: 0 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 @@ -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
18 changes: 18 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 @@ -4358,3 +4358,21 @@ 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

Copy link
Author

Choose a reason for hiding this comment

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

sure thing! I was trying to pass in the cartesian product of the all_connectable and dtype_backend arrays using itertools.product to @pytest.mark.parametrize but was running into issues with the connections getting passed. I instead made it so the connectables are being passed in the parametrize and then we loop through the dtypes in the test. Would love to know if there's a better way to do this so we're testing each dtype_backend/connection combination independently

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)
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

if dtype_backend == "pyarrow":
assert df.a.dtype == pd.ArrowDtype(pa.binary())
Loading