support both sqlalchemy engines and connections
State: closed

Fixes #7877


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

Comments:
This looks good and would also resolve #7877 I believe.
I think some tests will probably be requested.

@graingert Thanks for working on this!

Certainly look also to the discussion in #7877. At the end there is link to a patch of @tr11 which does something similar.

Some quick thoughts:
- indeed, as @artemyk mentions, this needs some tests. 
- The current tests are failing with both "ImportError: cannot import name interfaces" (I think using ``sqlalchemy.engine.Connectable` should work for all versions), and with an error in SQLAlchemyTransaction itself. You can see the output on the travis log (click the red cross above next to the commit to see the logs)
- I think also this will not yet work for `to_sql` and `read_sql_table`? (how you implemented it now, but is should work for those as well)

@graingert Some of the tests keep on running. I already restarted them a few times, so it seems this is caused by this PR. Did you try to run the tests locally? (only the sql tests is enough)

@jorisvandenbossche I've debugged all the tests.

Once the travis builds pass I'll remove the commits `Fix TestXMySQL` and `increase verbosity so I can see what's going wrong`

I think `Fix TestXMySQL` should probably go in another PR

@jorisvandenbossche @artemyk I can't replicate the issues in  4b2f087...1037144 locally do you have any ideas?

@artemyk @jorisvandenbossche I'm pretty sure I've got it now. 

I'll clean up this PR with a repo tomorrow

When running with two queries at the same time, if one raises a warning and the other doesn't PyMySQL raises a TypeError.

``` python
        proc = """DROP PROCEDURE IF EXISTS get_testdb;
         CREATE PROCEDURE get_testdb ()

         BEGIN
             SELECT * FROM test_procedure;
         END"""
```

The DROP PROCEDURE was raising a warning on Travis because the get_testdb procedure was missing, however on my local machine I was not destroying/rebuilding the database each test run which meant I still had the "get_testdb" procedure.

@graingert Glad you've figured it out. I'm not sure `execute` necessarily supports multiple statements, for example some DBAPIs (including PyMySQL if I'm not mistaken) have an `executemany` method for this purpose.

@artemyk executemany is for executing one statement with multiple parameters, not multiple statements.

@graingert Thanks for the updates! some questions:
- What was in the "Fix TestXMySQL" commit? 
- The problem with the stored procedure, this is only with a connection, not with an engine? In that case, maybe good to keep the original test for engine only (skip when connection), in addition to your adapated test.
- What was the problem with the pymysql version exactly?

Update: I see that you made a new PR for the first point:  #10230

XMySQL is only a problem on my local machine, I've got another PR for that
fix

Using pymysql 0.6.6 this error occurs in whichever test case runs first so
it's fine to run like this on the engine and the connection.

Using pymysql 0.6.3 'has various bug fixes' so I'm not sure what the actual
bug was.

There was a problem before with pymysql 0.6.6, see https://github.com/pydata/pandas/issues/9608 (but could never reproduce this locally)

Not sure what that issue is. This will work on 0.6.3 as well

So if I understand correctly, the problem with the `test_read_procedure` test is caused by pymysql (it works for me with pymysql 0.6.2, but errors with 0.6.6). 
Is there a reason to change the pymysql version on travis? (your second commit) Or can we keep the older version there untill this is fixed on the pymysql side, and then keep the test as it was before.

There's two bugs. One with pymysql 0.6.1 (dodgy cursors) and one with pymysql 0.6.6 (bad multi statement warnings).

Ah, yes, but you said that with 0.6.3 the first one (dodgy cursors) is also solved? Then maybe use 0.6.3 instead of 0.6.6, and then the procedure test does not need to be changed?

@jorisvandenbossche yeah looks like it works fine locally with 0.6.3

@jorisvandenbossche yeah the Travis tests pass on 0.6.3 what's your thoughts on a merge?

@jorisvandenbossche _casual nudge_

@graingert Thanks for the nudge :-)

This further needs:
- a notice in the whatsnew file (see doc/source/whatsnew)
- update to the docs: both all docstrings and the tutorial docs (see doc/source/io.rst)
- I think we should put in an explicit test that the desired behaviour works (and keeps working). Now it is only tested that the existing tests also run with passing a Connection, but we should add a test (only for when using Connection) that the example you give in the issue your reported works)

pls rebase on master, some issues have been fixed with the builds

@jorisvandenbossche done @jreback yep, rebase fixed those issues thanks.

@jorisvandenbossche @jreback _casual nudge_

@jorisvandenbossche @jreback hey guys, I really need this feature in production... can you guys merge this if you think it's useful

@graingert lgtm, but @jorisvandenbossche may have some more comments.

if you are actually running off of master (in production)?, then you can just merge this into your local master. This would be in 0.17.0 which won't be out for prob 2 months.

Sorry for the slow response. But I will look at it this evening, I promise!

Short comment: ideally, I would like to see a test that now fails (so a test that is only run for the test class that uses a connection, and not an engine), to be sure that with a later refactor this ability is not lost again.

@jorisvandenbossche that test is added here: https://github.com/pydata/pandas/pull/10105/files#diff-052a926a9b2c33a84e27d51c8fad3466R1372

Yes, I saw that, but if I understand correctly, this test is ran for both the engine tests as the connection tests? 
I was thinking that this enhancement adds something that was not possible now with engines (a use case for which you need the connection, and can't use the engine)? Eg using a temporary table. So it would be useful to have a test with such a use case, so we ensure that such functionality keeps working.

Ah I see what you mean