ENH: sql support via SQLAlchemy, with legacy fallback
State: closed

Code, tests, and docs for an updated io.sql module which uses SQLAlchemy as a DB abstraction layer. A legacy fallback is provided for SQLite and MySQL. The api should be backwards compatible, with depreciated names marked as such.


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

Comments:
@mangecoeur 

need to rebase away the merged branchs (makes history cleaner)

do this:

```
git fetch upstream
git rebase -i upstream/master
```

delete the tracked branch

```
git push upstream thisbranch -f
```

can also combine/squash branch as you see fit (you can do this later if you want)...post comments

https://github.com/pydata/pandas/wiki/Using-Git

@mangecoeur instead of `unittest.SkipTest`, use `nose.SkipTest` (these fail in py2.6)

@jtratner , @hayd you guys were involved at the design stage for this, is there something
we need to be aware of before moving forward based on @mangecoeur 's work? any
remaining issues that need to be ironed out before we commit to an implementation?

@mangecoeur , please update the optional dependencies secion of the README and
set a reasonable minimum on the supported version of SQLAlchemy. Nothing that would
force most users to upgrade nor get us tangled in a legacy whack-a-mole, if possible.
if 0.9.1 is not it, One of the builds should test with the minimum version.

@jreback, should we ask for a @sql decorator on these tests, what do you think?

also pls edit install.rst with the same requirements

u may want to test with an older version of sqlalchemy that is popular
(and make one of the requirements. for that)

Ok I slept on it, I now agree that automatic datatime conversion needs to be part of this feature.

I will try testing with SQLAlchemy 0.8 , I think that's probably the most widespread version. I don't anticipate any issues, their core api is stable.

Please add any more comments/suggestions and I will implement them in the coming week or so.

@mangecoeur, please don't merge master into your PR branches.  

Sorry, didn't realize, was just doing the workflow i was used to... can try
to revert if its a big problem...

On Monday, January 20, 2014, y-p notifications@github.com wrote:

> @mangecoeur https://github.com/mangecoeur, please don't merge master
> into your PR branches.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/pydata/pandas/pull/5950#issuecomment-32801129
> .

Legitimate, but we're settled on using rebase in pandas.  

ref to #5936

@mangecoeur can you rebase and squash a bit......

my brief look says that this does not break API's...

thoughts on inclusion in 0.13.1 ?

@y-p @hayd @danielballan 

I don't see any problems, but I'd like to see it sit on master for at least a week (two weeks?) before it's rolled into a release. Just one man's opinion though. How firm is that 6-day timeline on the 0.13.1 milestone?

@mangecoeur also need to remove all tracking/merge branches

see https://github.com/pydata/pandas/wiki/Using-Git

This is a big feature, which I would normally associate with a major release.
It hasn't lived in master for a couple of months, which I'd say is required to get it stable.

If you'd like to ship it as experimental in 0.13.1 and if the old code is still working, 
No strong objection here.

@danielballan will be at least that long....0.13 really just rolled out...

The point of quickly releasing 0.13.1 is to ensure a stable version is available (see various
perf regressions fixed) during the longer release cycle of the next major release (0.14), so we 
can _start_ merging big features into master.

That's the idea anyway.

OK, just basing that on the milestone. Then I don't see a problem.

Has anyone done benchmarks on SQLA-mediated connections vs. MySQLdb / sqlite as it's set up now? I volunteer, once it's in master.

@y-p is right here....(I wanted the docs updated)...to reflect 0.13 changes...

maybe can split off the doc updates to satisfy #5936 then roll this to master right after 0.13.1

I am sure that the dtype inference needs some more work and that will happen in 0.14 (hopefully)

It's become obvious that the RC's really don't get that much extra testing. 
Users wait for the official release. So we only get the bug reports right after a release
which we can then address in a point release. hopefully quickly.

I'd love a better solution, do you have a hypnotic ray I can borrow to get people to test our RC's?

Squashed the history into 3 commits with more or less informative messages (not so used to using git this way, but hopefully shouldn't have screwed anything up)

Let me know how to finish this to get this merged to master, it certainly needs to be exercised in the real world to see how people get on, this should generate some new issues for improving in the next release.

On the docs - because they've been changed quite a bit to cover sqlalchemy and the read_table function I don't think it makes sense to split them out as they stand. I feel confident we could push the whole thing to master and see how people get on.

How do we envisage the user API? 
In the docs, at the moment there is `from pandas.io import sql` and then `sql.read_sql/to_sql/read_table`
- What do we want top-level? At the moment there is `pd.read_sql` and `pd.DataFrame.to_sql`. Is this to stay? Should it then be used like that in the docs? (but then it contrasts with `sql.read_table`)
- What other functions do we regard as public? 
  - There were votes against `sql.tquery` and `sql.uquery` and this has been removed from the docs and deprecation warnings added , so OK.
  - `sql.has_table`
  - `sql.execute` seems also useful? If so, also mention it in the docs?

To finish summary: also deprecated are `sql.read_frame` and `sql.write_frame` (they should be removed from api.rst)

I'm not sure if it make sense to put in 0.13.1 in case something is broken.

read_table is confusing name if brought into global... I think read_table should be called read_sqltable or better yet should just be part of read_sql (if you give a table name as sql/first argument).

what does `read_table` actually do (as opposed to `read_sql`)?

`read_table('people')` does the same thing as `read_sql('select * from people')`. We definitely can't use `read_table` globally -- that name is already being used for something non-SQL-related. I like @hayd 's suggestion: build this into `read_sql`. If the first argument matches the name of one of the columns, the meaning is unambiguous.

I am also in favor of including `read_table`'s functionality into `read_sql` (with guessing or with a `table` keyword). 

This also solves the discrepancy of top level `pd.read_sql` but in submodule `io.sql.read_table`. The other more 'specialized' functions (`has_table` and `execute`) can then reside in `io.sql`.

@danielballan the two are not quite equivalent, since the read_table function selects from a table reflected from the database it can do much better type conversion than read_sql('select \* from table'). We can't easily get the expected types from an sql statement without basically guessing from the returned values. Pandas does a good job of figuring it out, but Because read_table is limited to querying existing tables with known column types we can ensure the dataframe columns match the sql ones. Dive into `_harmonize_tables` to understand the difference.

I also kept read_table separate because it's much easier to extend without cramming potentially mutually exclusive optional arguments. You'll see I already added a columns option so you can do e.g.
read_table('people', engine, columns=['firstname','age'])
You get the columns specified with correct type convertion. And theres lots of room to add to this in the future e.g you make joins possible, all fully db agnostic.

Merging with read_sql would result in a horrible mess of optional arguments that are valid or not depending on whether you give a sql statement or a table name, on top of the legacy support stuff already there. Thats just ugly design. Its much better to have two functions each with a clear and well defined scope than to ill advisedly cram all your functionality into one monster function. Its easier to read, understand, document, test, and debug - sql.read_sql reads an sql statement, sql.read_table reads a table from an sql database. They represent different levels of abstraction.

If read_table must be made top level (tho imho there's not much advantage to this) then it could be named read_sql_table instead.

A more general remark on the code: can you try to check for pep8 (eg I saw some long lines)?

Something else: I have a bunch of comments on the documentation/docstrings. Do you have time for that at the moment (if I can put them here tomorrow or so), or do we leave that for another PR? (I can also try to do a PR against your branch)

@mangecoeur BTW, this is really a nice improvement for pandas!

I realise it's less elegant and I think we should definitely keep read_sql_table inside sql, for explicitness. But this way we wouldn't have to export it globally (via the io api).

All it takes is:

```
def read_sql(sql, ...):
    if has_table(sql):
        return read_sql_table(sql, ...)

   # previous logic
    ...
```

Granted there are some more arguments to play around with, but I think this would work well, as is inline with other pandas functionality.

You can use [autopep8 on the sql files](http://stackoverflow.com/a/14328499/1240268) (sql and test_sql)

```
autopep8 py_file --in-place --pep8-passes 2000 --verbose
```

.

Just to confirm, what test are being run and passing?
- All previous sql_legacy tests ?
- New test class for all of (sqlite old way and engine, mysql old and engine, postgres?) ?

Are they all being run by travis?