SQL: bunch of fixes based on the old tests #6987
Conversation
|
look at the numpy master build ; its going to by default show all DeprecationWarnings I think that we use 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 |
|
@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, 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 (jorisvandenbossche@326c973). For |
|
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 |
- adapt signature of get_schema to what it was before (for backwards compatibility), possibility to call function without connection object - fix get_schema for sqlalchemy mode - add tests
As tquery and uquery are deprecated, it is not needed to adapt them to the new sqlalchemy interface + the adapted version changed the signature slightly so it was not fully backwards compatible. Therefore: put back the old code and removed new implementatation in the classes. Consequence: the functions are not available for the new sqlalchemy engines, but as they are deprecated this is not needed.
|
Added the old tests back for now (to ensure they are passing). OK?
|
|
@danielballan Do you have time to look at the execute cursor issue? For that, the |
|
@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) |
| except Exception as e: | ||
| excName = e.__class__.__name__ | ||
| if excName == 'OperationalError': # pragma: no cover | ||
| print('Failed to commit, may need to restart interpreter') |
There was a problem hiding this comment.
shouldn't this raise? (and not print)
There was a problem hiding this comment.
It should not raise I think, as it has to be able to 'retry' a couple of lines lower.
Now I don't fully grasp this code, I just copied it from 0.13.1 version to retain backwards compatibility (Wes put this in 2 years ago), so I thought: no real need to clean this up, as we will remove it anyway after a deprecation cycle.
There was a problem hiding this comment.
ok....that's fine...just checking, prob right not to touch it!
There was a problem hiding this comment.
But I can make a warning of it instead of a print statement
|
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 |
- removed meta keyword from has_table (was not used) - added check for if_exists - added table_exists back as alias for has_table - added frame_query back as alias for read_sql (and deprecated it) - changed way to delegate read_sql/table without using has_table for backwards compatibility (because this gave an error when uqing a mysql connection but not specifying the flavor) Things that I had to change to the tests to get them running: - changed MySQLdb to pymysql - changed `sqlite3.OperationalError` to `pd.io.sql.DatabaseError` (in `test_tquery` and `test_uquery`) - test_write_row_by_row are skipped for now (they depend on the issue of `execute` being able to handle cursor objects). - in `test_execute` with pymysql converted the row values to python types (with numpy's `tolist()`), as pymysql errored on the numpy types.
|
@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 if you are eager to review and pull it in so this can merge. Edit: Make that here. 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: 043d165 (see also comment in 4th task point in #6292). Apparantly the tests were failing for some reason. And @mangecoeur changed it from MySQLdb to pymysql in the big refactor here: 2e3f83d (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 |
|
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 |
|
@danielballan OK, I picked your fixed, tests are passing now. Thanks! I am going to merge this. |
SQL: bunch of fixes based on the old tests
Some first fixes based on running the old sql tests (from 0.13.1 tag)
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 forget_schema)table_existsseems to be renamed tohas_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:
tqueryhad aretryandcurarguments (which were at least used in the tests), but not that simple to put back with the new implementation@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?