Add SQL Support for ADBC Drivers
State: closed


Labels:
name: IO SQL
description: to_sql, read_sql, read_sql_query

Comments:
AFACIT the pyarrow drivers required pyarrow>=8.0.0 since they use the RecordBatchReader object, but our min version is still pinned at 6.0.0. @phofl I know you've upgraded us in the past with pyarrow - is it too soon to make the jump to 8.0?
We could likely relax Python and PyArrow minimum versions further if needed
Technically we could upgrade for 2.1, but this depends on pdep 10 as well
What is the link between this and PDEP 10? The ADBC dbapi requires pyarrow to use but I don't think that impacts how we need to manage that on our end? Or am I overlooking something simple?
PDEP10 has some language around minimum supported Arrow versions
But note that the PDEP-10 text is even more conservative, so based on that we would not be bumping to pyarrow 8 as a minimum version any time soon (only next year).

> AFACIT the pyarrow drivers required pyarrow>=8.0.0 since they use the RecordBatchReader object, but our min version is still pinned at 6.0.0. 

IMO there is really no problem in requiring a more recent pyarrow version for certain new features, such as pyarrow 8 for using adbc in our SQL IO, while the general minimum required version is lower.
> IMO there is really no problem in requiring a more recent pyarrow version for certain new features, such as pyarrow 8 for using adbc in our SQL IO, while the general minimum required version is lower.

Yea I agree. That definitely adds complexity to test compat/xfailing, but I think can be reasonably implemented in another pass at this
This PR isn't stale - I am just holding off until the 0.6.0 drivers get released as they have a lot of updates that give us better type coverage. I ultimately plan on setting that as the min version for pandas support
Sorry - that's been held up a long time because of waiting for Arrow 13
Oh yea no worries - not a rush at all. 
This is getting pretty close. Skipped tests where appropriate to try and get green. We might want to again bump to 0.7.0 as a minimum which should get round out remaining issues (a lot of skips come back to column escaping / capitalization).

The drivers look pretty good though. I think most of the skips in the test suite are just things that we would need to implement in pandas, likely in follow ups
Mentioned on the last DEV call but I think its worth waiting until ADBC 0.8.0 and setting that as the minimum; it's not a huge deal but it would make our test suite a little cleaner having proper BOOLEAN support in the driver. I also assume that the 0.8.0 release of ADBC will be out in the next month or so and pandas 2.2 will be out in a few months, so thinking there is no rush.

I'll  be presenting on this at PyData 2023 in NYC. I've put up a rough sketch of what I'll be talking about for those interested: https://github.com/WillAyd/pydata_nyc_2023-talk/blob/main/pydata_nyc_2023.ipynb

My basic benchmarks show this as ~35x faster for postgres writes than our default today. If you use `method="multi"` (which I mistakenly assumed would be faster) the difference is ~185x 
Looks like https://github.com/pandas-dev/pandas/pull/55591 is a pre-cursor to getting CI green on this now
> Mentioned on the last DEV call but I think its worth waiting until ADBC 0.8.0 and setting that as the minimum; it's not a huge deal but it would make our test suite a little cleaner having proper BOOLEAN support in the driver.

Now being voted on! Should wrap up next week.
I see #55591 is merged but still getting the warnings on this PR. Is that expected @jorisvandenbossche ?
Ah, yes, that PR ensured that the warning wouldn't be user visible, but in our CI we still ensure every warning is turned visible and turned into an error, so you still need to explicitly ignore the warning (so actually it didn't need to wait on that PR for this). 

You can do the same as in some of the other pyarrow-related IO tests, i.e. ignore them for the full file with:

https://github.com/pandas-dev/pandas/blob/09ed69edc3a83141244b3b7e1098d0083c8a9440/pandas/tests/io/test_orc.py#L20-L22


For getting this merged, can you add a section about this in the whatsnew file? (and maybe reuse the same content to mention it in the user guide (io.rst) as well)
Is there a trick to the doc build with conda? I don't see it installing the ADBC drivers even though they are in environment.yml
If we want to promote using ADBC prominently in the docs, we should maybe make it easier to use and also allow passing a connection URI in addition to a connection object?

So one can do `pd.read_sql("SELECT ..", con="postgresql://....")` ?

The only question is then how to distinguish this when to use SQLAlchemy vs adbc. SQLAlchemy allows to specify the specific driver by using "postgresql+psycopg2://...". So maybe we could allow the user to do "postgresql+adbc_driver_postgresql://...", and then we could easily detect that the first part of the URI contains "adbc"?
> The only question is then how to distinguish this when to use SQLAlchemy vs adbc. SQLAlchemy allows to specify the specific driver by using "postgresql+psycopg2://...". So maybe we could allow the user to do "postgresql+adbc_driver_postgresql://...", and then we could easily detect that the first part of the URI contains "adbc"?

As far as I know the `postgresql+adbc_driver_postgresql` parsing is a SQLAlchemy feature, where it parses out the dialect and driver. At least I couldn't find mention of this elsewhere outside of the [SQLAlchemy docs](https://docs.sqlalchemy.org/en/14/dialects/#external-dialects).


Alright all green and I think I've address comments. Let me know if anything is missing
OK all green and I think I addressed your feedback @mroeschke 
Nice! Thanks @WillAyd. Can have follow up PRs if needed