-
- Notifications
You must be signed in to change notification settings - Fork 19.3k
Fix BUG: read_sql tries to convert blob/varbinary to string with pyarrow backend #60105
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
Changes from 2 commits
2244869 bd00fc5 6a23f05 2f261c8 8f900b8 536b1ed 8965a1d a32b4a6 f412c72 a0200d0 10ca030 ba2e82d 8dd45ca 602194a f8ae285 02514e0 ef5c2ed da7817a de5c604 e025d84 3d83bab 6ea5785 88accb3 c066ebf 2afeed5 de286a4 a99365a a8f69e5 2f8ac63 1a0b2cf 1da6ee4 4c1c282 fe4bdba 71d94e5 8813f77 30ce3b3 9721fc3 bcb7e3d fe52a7a File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -970,7 +970,17 @@ def convert(arr): | |
| if dtype_backend != "numpy" 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) | ||
| try: | ||
| # Addressing (#59242) | ||
| # Byte data that could not be decoded into | ||
| # a string would throw a UnicodeDecodeError exception | ||
| | ||
| # Try and greedily convert to string | ||
| # Will fail if the object is bytes | ||
| arr = arr_cls._from_sequence(arr, dtype=new_dtype) | ||
| ||
| except UnicodeDecodeError: | ||
| pass | ||
| | ||
| elif dtype_backend != "numpy" and isinstance(arr, np.ndarray): | ||
| if arr.dtype.kind in "iufb": | ||
| arr = pd_array(arr, copy=False) | ||
| | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -4352,3 +4352,18 @@ def test_xsqlite_if_exists(sqlite_buildin): | |
| (5, "E"), | ||
| ] | ||
| drop_table(table_name, sqlite_buildin) | ||
| | ||
| | ||
| def test_bytes_column(sqlite_buildin): | ||
| pytest.importorskip("pyarrow") | ||
| """ | ||
| ||
| 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' | ||
| """ | ||
| query = """ | ||
| select cast(x'0123456789abcdef0123456789abcdef' as blob) a | ||
| """ | ||
| df = pd.read_sql(query, sqlite_buildin, dtype_backend="pyarrow") | ||
| assert df.a.values[0] == b"\x01#Eg\x89\xab\xcd\xef\x01#Eg\x89\xab\xcd\xef" | ||
| ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.ArrowExtensionArraywith._dtypeofpandas.ArrowDtype. When the query returns a bytes type column we get a.typeofbytes, and likewise a.typeofstringis 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 apyarrowdtype_backendwith theSQLiteDatabaseconnection type here: https://github.com/pandas-dev/pandas/blob/main/pandas/io/sql.py#L695 ?There was a problem hiding this comment.
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.pyand 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
There was a problem hiding this comment.
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_sequenceshould be able to return a binary type with binary data.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in
sql.pyit looks like the result of this conversion is being overwritten when thedtype_backendispyarrow: https://github.com/pandas-dev/pandas/blob/main/pandas/io/sql.py#L181 and the dtype returned by the current logic isArrowDtype(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 ofArrowDtype(pa.binary())when thedtype_backend='pyarrow'