BUG: When creating table, db indexes should be created from DataFrame indexes
State: closed

Unfortunately my PR #8022 introduced a bug wherein database indexes are not created from DataFrame indexes.  This pull request fixes it, also introduces this feature for the legacy interface, and adds tests to make sure indexes are created as appropriate.


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

Comments:
Can you be a bit more clear of what you think the bug is? (eg give an example of the buggy behaviour).

On explicitely creating an INDEX from the pandas indices, this is a not implemented feature at the moment, as now, the index is just written as any other column (slightly related: #7984). 
So I am not sure you introduced a bug, but we should think about what API we want to provide to better specify what should happen with the index.

Here's an example:

``` python
import sqlalchemy
engine = sqlalchemy.create_engine('sqlite:///:memory:')

import pandas as pd
from pandas.io.sql import PandasSQLTable, PandasSQLAlchemy
df = pd.DataFrame.from_records([{'a':1,'b':2.1,'c':'txt1','a':2,'b':1.5,'c':'txt2'}], index=['a'])
df.to_sql('test_df', engine)

print pd.read_sql_query("SELECT * FROM sqlite_master WHERE type='index' AND tbl_name='test_df'", engine)
```

Currently on master this results in:

```
Empty DataFrame
Columns: [type, name, tbl_name, rootpage, sql]
Index: []
```

if I revert to before the commit for PR #8022 (or after the commits in this PR) the output is:

```
    type          name tbl_name  rootpage  \
0  index  ix_test_df_a  test_df         3   

                                        sql  
0  CREATE INDEX ix_test_df_a ON test_df (a)  
```

Ah, yes. Sorry, I missed we already did `index=True` in the `Column` creation for the index (was confusing it with unique/primary key feature requests). Thanks for the clarification :-) 
Indeed you removed it: https://github.com/pydata/pandas/pull/8022/files#diff-b41f9fd042c423682f8e4c4d808dbe64L680 
OK, will look at this later.

Added explanation to executescript.  Unfortunately tests are not passing because `execute` is now returning None results for legacy MySQL mode under some conditions.  Will investigate this a bit later... any ideas?

BTW, you should also update the test_get_schema (pandas.io.tests.test_sql.TestSQLLegacyApi) test

@artemyk I have not directly an idea why it would fail for legacy mysql, but an idea to solve it: only implement it for sqlite. 
For fallback it is actually a new feature (was never there, opposed to sqlalchemy mode), and the `mysql` flavor is deprecated. Only calling this when the flavor is `sqlite` will also simplify the code a lot, as you don't need the execute/executescript hack. Only two times a `if flavor ==` check to do different things.

@jorisvandenbossche I realized that the tests were failing because of a bug in pymysql 0.6.1 wherein calling `.execute` on a SQL string with multiple statements causes a mismatch in returned cursors.  See https://github.com/PyMySQL/PyMySQL/issues/157 .  This was fixed in pymysql 0.6.2.

I know we are moving away from legacy mysql support, but I feel that as long as the support is there, we should make it as consistent as possible (this is also my thinking for adding index creation to the legacy APIs --- to try to keep it consistent with the sqlalchemy API, though I realize this is actually an enhancement).  It seems strange to me if legacy sqlite did create indexes in `to_sql`, but legacy mysql didn't.  What do you think about upgrading the pymysql requirement to 0.6.2 ? In this  case, the tests for this PR would pass -- as they do for me locally with 0.6.2 -- and both sqlite and mysql would have the same behavior.

For the sqlite fallback (or 'legacy', but that is actually a bad name), it is indeed important to keep it as consistent as possible, as this will stay in the future as 'fallback'. So in this regard, your work to add it INDEX to it in this PR is really good. 
The mysql legacy will be removed in future versions, and is only there for backwards compatibility for the time being, so I don't think we should put time in improving it. That said, of course, if it's gets better by improving sqlite support, that is not a problem. However, in this case, it makes it only more complicated I think. Plus, `pymysql` 0.6.2 is rather recent (april this year, so only couple of months). So in that case, people using the deprecated mysql, will get maybe a 'better' `to_sql` function in pandas 0.15, but probably have to update their pymysql version to be able to use it. So in that regard, I would rather keep it 'as is' for mysql (raising the required version for something that is deprecated is a bit strange I think). What do you think? 
Of course, you could do a check for the pymysql version, and only doing it if the version is high enough, but that effort is not worth it I think.

or another option: instead of using executescript, just using seperate execute statements for creating the index. Is that possible?

@jorisvandenbossche That's a good idea --- I changed it to generate multiple statements and everything seems to work.  
Implementing a test for `get_schema` and realized that `_get_schema_legacy` and `_create_table_statement` seem to be duplicating a lot of the same code.  Shouldn't they be combined?

Also, while I'm working on this code --- right now, if there is a multiindex on, say, columns `A` and `B`, then we create one index on column `A` and one index on column `B`.  It seems semantically much more appropriate to actually create an index on `(A,B)` (ie, both columns at once).  Is this something that would make sense to change?

@artemyk yes, I was also wondering that when reviewing your changes, but I don't really know what would be best. As having a multi-column index, this also restricts searching on the not-leftmost index compared to having seperate indices (as far as I understand).

For the duplication in `_get_schema_legacy`, I think this was originally done for mysql, because in principle you need a connection object to use the `PandasSQLTableLegacy._create_table_statement` function (and `get_schema` does not need a connection in legacy mode, it does need an engine when using sqlalchemy). But maybe this can be circumvented by 'fooling' it by giving it an in-memory sqlite connection.

I will look around but I do think it shouldn't be too hard to let `PandasSQLTableLegacy` be instantiatable without a connection.
Regarding multiple indexes -- yes, creating a two column index (let's say `(A,B)`) would make searching on `B` slow (non-indixes).  On the other hand, create two one-column indices (as ew do now) would make searching on a conjunction of `A` and `B` slow (e.g. `A=='txt' AND B==10`).  I think the latter corresponds closer to how a multiindex functions in pandas.  I think if a user wants to have extra indices on, say, individual columns, perhaps this can be an extra option to `to_sql` or they can create those themselves after the fact.

for the `get_schema` issues, lets leave that for another PR. I opened an issue for that: #8097, so we can discuss further over there

I actually asked about the get_schema issues because it does not currently return `CREATE INDEX` statements so tests on get_schema will fail.  I will remove `get_schema` tests from this PR and add it to #8097 .
Regarding multiindexes create multiple-column indices, if you think it's something that should be fixed, then I can create a separate PR for it.

These commits should incorporate the latest round of changes.  I renamed `_create_table_statement` to a less confusing `_create_table_setup` since in the sqlalchemy case it actually returns a sqlalchemy table object and in legacy case it returns a list of statements.

Looks good (one remark, see above), can you squash? 
And when you squash, can you add a reference to the issue in the commit message? (normally the issue number, but here the PR number is OK as there is not really an issue: `(GH8083)`) Ideally in the first sentence.

@artemyk I merged #8090, so you will have to rebase and possibly solve the merge conflicts.

@artemyk Do you have some time to rebase and squash this? I would like to get this in.

@jorisvandenbossche Sorry for the delay, was on travels.  Rebased on upstream master and squashed commits.   Please let me know if there's anything else. 

@artemyk No problem!
Just one small comment.

Test fixed.

@artemyk Thanks a lot! I will try to look at your schema PR this evening.