SQL: bunch of fixes based on the old tests
State: closed

Some first fixes based on running the old sql tests (from 0.13.1 tag)
- fixed `get_schema` (and adapted is signature back to how it was for 0.13.1). For this I added back the old function, as this functionality was not possible with the refactored class based approach (for those you always have to provide a connection object or engine, this was not necessary for `get_schema`)
- `table_exists` seems to be renamed to `has_table`? I don't know why (and we should also decide if we expose this as a public function), but added it back for now as an alias.

All tests from the old test suite are now running with the new code (for sqlite, not yet tested for mysql), except for the following issues:
- `tquery` had a  `retry` and `cur` arguments (which were at least used in the tests), but not that simple to put back with the new implementation
- OperationalError is changed to DatabaseError

@jreback I also changed the DeprecationWarnings to FutureWarnings (I am right this is the canonical way to raise deprecation warnings in pandas?) and catched them in the tests. I don't know if this solves #6984 for the sql warnings?


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: IO SQL
description: to_sql, read_sql, read_sql_query

Comments:
look at the numpy master build ; its going to by default show all DeprecationWarnings

I think that we use `FutureWarning` (but numpy uses `DeprecationWarning`) odd

https://travis-ci.org/pydata/pandas/jobs/23890714 (not done yet)

I think it did get rid of 1-2 warnings (1 left for sql, buy you said ok)

@jreback Yes, the one warning that is left is the warning of not using sqlalchemy (user warning, no deprecation warning). If we keep it, we should catch that too.

I think we use `FutureWarning` because `DeprecationWarning` is not printed by default for the user? Seems like a good reason to use it.

@danielballan While getting the last tests of the old test suite running (the ones that were using tquery/uquery), I stumbled on the following issue:

Previously, `execute` had a `cur` keyword (https://github.com/pydata/pandas/blob/v0.13.1/pandas/io/sql.py#L32), while now this is removed: https://github.com/pydata/pandas/blob/master/pandas/io/sql.py#L90  (this happened in your commit https://github.com/pydata/pandas/commit/74d091f95ec293c195bd0a5e4886cf5891df2489) (it says 'deprecated', but it is actually just removed). 
This has following consequence: previously you could use `execute` to do consecutive inserts via the same cursor. This was used in the tests (https://github.com/pydata/pandas/blob/v0.13.1/pandas/io/tests/test_sql.py#L73, actually using `tquery`, but this boils down to `execute`), and this now fails as this behaviour is no longer possible (executing on a cursor instead of connection).

Was there a reason to remove it? (of course, if you want to do multiple inserts, you should just write your dataframe, but is _was_ possible before)

I don't know. Actually, most of those commit messages in 74d091f are not mine. I remember that @mangecoeur did a major rebase on the re-write PR, crediting me (and others) with an arbitrary subset of the commits to give me credit for a share of the effort. There were parallel efforts between @hayd  and @jtratner as well, I think. Those commits could be any of ours -- maybe one of them remembers the reasoning.

I notice the commit message, "IMPORTANT - changed legacy to require connection rather than cursor. This is still not yet finalized." It seems this was overlooked. Let me see if it would be hard to change it back without breaking the new stuff....

@danielballan Great if you could look at it.

Can you also have a look at the third commit here (https://github.com/jorisvandenbossche/pandas/commit/326c973310eb2035c790b6ec2a33deb6ed36d6d8). For `tquery/uquery`, I just removed the current implementation and put back the one from 0.13.1, because: a) we deprecate it, so no need to add it to the sqlalchemy mode, and b) the new implementation was not fully backwards compatible (signature changed a bit) for the legacy mode.

FYI, travis just changed to support 3.4 and broke all of our builds...fix coming in a few

has_table is the name of the function from sqlalchemy, I expect this is the reason for the change. We could alias and depreciate table_exists?

It seems strange to delete the cur kwarg, IIRC the old sql code was _purely_ using cursors (if you passes a connection it just unwrapped the cursor) and being able to pass a cursor is part of the DBAPI standard (?). As mentioned all my code was completely rewritten or perhaps squashed, so I'm not sure here... I think should bring it back / depreciate in case it's being used in the wild.

FYI travis is good to go now

Added the old tests back for now (to ensure they are passing). OK? 
Things I had to change:
- changed MySQLdb to pymysql
- changed `sqlite3.OperationalError` to `pd.io.sql.DatabaseError` (in `test_tquery` and `test_uquery`)
- in `test_execute` with mymysql converted the row values to python types (with numpy's `tolist()`), as pymysql errored on the numpy types.
- `test_write_row_by_row` are skipped for now (they depend on the issue of `execute` being able to handle cursor objects as dicussed above)

@danielballan Do you have time to look at the execute cursor issue? For that, the `test_write_row_by_row` tests should then pass (https://github.com/jorisvandenbossche/pandas/commit/7f327ea695f2e414524159c49405e732690070f4#diff-052a926a9b2c33a84e27d51c8fad3466R1162), so the `SkipTest` should be removed then.

@jreback the tests are passing now, except for the ones testing for FutureWarnings (I thought they did pass before, and locally they also pass). Do you have any idea why they are not passing? Because the FutureWarnings are really in there.

I think this is almost ready. @jreback @danielballan @hayd @mangecoeur Somebody able to review? (maybe easier to look at the changes for each commit seperately, as they are not all connected)

@jorisvandenbossche 

I did this to test the deprecations of float indexers; I couldn't get it work otherwise (turn the warnings into exceptions, assert they raise), turn it back off

https://github.com/pydata/pandas/blob/master/pandas/tests/test_indexing.py#L3255

I think their is some general turning off of FutureWarnings somewhere which prevents it from working properly

@hayd @danielballan any comments? If not I will merge tomorrow to further work on finishing the sql tasks for 0.14.

I can't find why we changed from MySQLdb to pymysql. Shouldn't matter, I guess, but what's the reasoning?

@jorisvandenbossche I worked on the execute cursor issue. Haven't tested, and I have to put it down for a moment, but you can get it [here](https://github.com/danielballan/pandas/commit/9bc09651fbb264dccdc5ad59729a9557aa060ea0) if you are eager to review and pull it in so this can merge.

Edit: Make that [here](https://github.com/danielballan/pandas/commit/d5798e87f8677adf473a793a2836e84d66d7533c). Minor fix.

@danielballan thanks for the fix for execute. I will look at it tomorrow.

About MySQLdb vs pymysql. I don't know exactly, but it was removed from the requirements by @jreback here: https://github.com/pydata/pandas/commit/043d165290180c6d9e3a480c18e220548af56c53 (see also comment in 4th task point in https://github.com/pydata/pandas/issues/6292). Apparantly the tests were failing for some reason. And @mangecoeur changed it from MySQLdb to pymysql in the big refactor here: https://github.com/pydata/pandas/commit/2e3f83df8a29f257373ebdc71ebc03f75253a987 (maybe that is the reason the tests were not working anymore, as in requirements it was still MySQLdb). 

BTW, it does matter in some way, as one of the tests were failing when I used pymysql (pymysql could not handle a numpy.int64 type, I had to add `tolist` here: https://github.com/pydata/pandas/pull/6987/files#diff-052a926a9b2c33a84e27d51c8fad3466R1473), and I assume it ran correctly before with MySQLdb.

Without knowing @mangecoeur's reasons, I'd say it doesn't make sense to move from MySQLdb to pymysql, especially if we're only going to remove it entirely in the next release. We should find out the reasoning before we put in any effort though....

we could certainly add MySQLdb on a build....easy enough

why don't you try to change one of the builds (it will just compile it inplace), see if it works, then I can build a wheel for it, e.g. change `ci/requirements-2.7.txt` (and replace `pymysql`)

@danielballan OK, I picked your fixed, tests are passing now. Thanks!

I am going to merge this. 
But for the MySQLdb, also for sqlalchemy we have to test it with mysql, so we are not going to drop tests with mysql. So it can be interesting to test multiple drivers if there are differences there.