Shouldn't this ideally returnpandas.ArrowDtype(pyarrow.binary()) type?

that makes sense thanks! so looks like the previous logic was not taking into account pyarrow types when doing this conversation so I've added logic similar to my initial change where we try to convert to a pyarrow string but then fall back to binary if we run into an invalid error (i.e. we tried to parse but it failed due to an encoding error). Please let me know what you think! Was also considering trying to type check the contents ofarr to see if it has string or bytes data but seems like greedily trying to convert ends up being better performance in most cases (since we might have to search the whole arr to see if one of the elements is a bytes sequence that can't be converted to a string)

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?

Based on some local testing using the ADBC driver I can confirm that it yields apandas.core.arrays.arrow.array.ArrowExtensionArray with ._dtype ofpandas.ArrowDtype. When the query returns a bytes type column we get a.type ofbytes, and likewise a.type ofstring 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 adtype_backend != 'numpy' since this will fix the cause of the exception in the issue? and maybe raise an exception when trying to use apyarrowdtype_backend with theSQLiteDatabase connection type here:https://github.com/pandas-dev/pandas/blob/main/pandas/io/sql.py#L695 ?

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 tosql.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

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

insql.py it looks like the result of this conversion is being overwritten when thedtype_backend ispyarrow: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'

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 thenumpy_nullable backend - object?

for sure, changed the testing logic over to using this! fornumpy_nullable andlib.no_default the dtype returned is an object

Should test this against all databases

sure thing! I was trying to pass in the cartesian product of theall_connectable anddtype_backend arrays usingitertools.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 theparametrize 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 eachdtype_backend/connection combination independently

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

Hmm why is this defaulting to object out here? That seems like something to avoid for the pyarrow backend.

I think you are doing that in this test anyway, but the structure is confusing. I would just set up the expected data type from the backend up front, so something like:

ifdtype_backend=="pyarrow":exp_dtype= ...else:     ...

makes sense, let me know if the modified structure is less confusing!

Why is this an opaque type instead of just binary? That seems like a bug somewhere in this implementation or the driver

this is coming fromadbc-driver-postgresql, I guess they've chosenopaque because this is a bit of a weird type in postgres.https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-BIT-STRINGS vshttps://www.postgresql.org/docs/current/datatype-binary.html

postgres=# select x'0011';     ?column?     ------------------ 0000000000010001(1 row)postgres=# select pg_typeof(x'0011'); pg_typeof ----------- bit(1 row)postgres=# select cast('0011' as bytea);   bytea    ------------ \x30303131(1 row)postgres=# select pg_typeof(cast('0011' as bytea)); pg_typeof ----------- bytea(1 row)

Hmm AFAIK the OpaqueType is returned when there is no suitable database type, but isn't binary already returned directly? It is a bit unclear what this additionalisinstance check is for

I had to add this becauseadbc-driver-postgresql is returning anOpaqueType - without this included an exception is thrown

Why does postgres have different content than the other databases?

thex'...' syntax in sqlite and mysql represents the same data (called a blob literal in sqlite ]) but in postgres this is slighly different in that it's not abytea type , but instead abit. Most of the drivers return this as a string representation of the bits (which is whyval = "0000000100100011010001..." in one of the cases)

Instead of selecting a literal value can you write and read back the dataframe? I think this PR is missing something about how the different drivers are going to return binary data.

The ADBC driver has maps binary data to the BYTEA type (see alsohttps://github.com/apache/arrow-adbc/blob/c6e529980f1369202adb101f2e1ea03c4e12dcdc/c/driver/postgresql/postgres_type.h#L599) so we should be round tripping that, not bits

What does#Eg represent?

this is the raw bytes returned from the hex string"0123456789abcdef0123456789abcdef" - I've changed the test to usebytes.fromhex("0123456789abcdef0123456789abcdef") to make things more clear here

Is this supposed to be the binary representation of the bytes? I'm a bit confused why this test has so many different expected values

yeah exactly and this is due to postgres returning thebit type which most of the drivers turn into this string

Instead of selecting a literal value can you write and read back the dataframe? I think this PR is missing something about how the different drivers are going to return binary data.

The ADBC driver has maps binary data to the BYTEA type (see alsohttps://github.com/apache/arrow-adbc/blob/c6e529980f1369202adb101f2e1ea03c4e12dcdc/c/driver/postgresql/postgres_type.h#L599) so we should be round tripping that, not bits