ENH: Support writing timestamps with timezones with to_sql
State: closed

- [x] closes #9086
- [x] closes #23510
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Enhancement
name: IO SQL
description: to_sql, read_sql, read_sql_query
name: Timezones
description: Timezone data dtype

Comments:
Hello @mroeschke! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/io/sql.py`](https://github.com/mroeschke/pandas/blob/befd2000f1a1a3b1c9b06aa348011ef2c23bf0e4/pandas/io/sql.py) !

 - There are no PEP8 issues in the file [`pandas/tests/io/test_sql.py`](https://github.com/mroeschke/pandas/blob/befd2000f1a1a3b1c9b06aa348011ef2c23bf0e4/pandas/tests/io/test_sql.py) !


# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=h1) Report
> Merging [#22654](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/73dd6ece3b66008eb8077098c101d0e300b1e714?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22654/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22654   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files         169      169           
  Lines       50708    50708           
=======================================
  Hits        46740    46740           
  Misses       3968     3968
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.58% <ø> (ø)` | :arrow_up: |
| #single | `42.37% <ø> (+0.01%)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=footer). Last update [73dd6ec...c7c4a7a](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jorisvandenbossche so looks like based on the passing tests, databases without timezone support store those the data as naive local time. 
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=h1) Report
> Merging [#22654](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/28a42da41ca8e13efaa2ceb3939e576d08c232c8?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22654/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22654   +/-   ##
=======================================
  Coverage   92.24%   92.24%           
=======================================
  Files         161      161           
  Lines       51224    51224           
=======================================
  Hits        47254    47254           
  Misses       3970     3970
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.63% <ø> (ø)` | :arrow_up: |
| #single | `42.3% <ø> (+0.02%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/22654/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.81% <ø> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=footer). Last update [28a42da...ef3b20f](https://codecov.io/gh/pandas-dev/pandas/pull/22654?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Thanks @TomAugspurger. Addressed your comment. 
If you could have a look as well @jreback 
lgtm. @jorisvandenbossche merge when ready.
@jorisvandenbossche took your suggestions and added a section in io.rst explaining datetime/timezone io with SQL
Since I can't seem to answer to my previous inline comment, repeating it here:

> I think it would be good to mention what this means in practice.
>
>So if one had now a tz-aware column, it was converted to naive UTC, and you got back naive UTC. So the difference is now that you will get back tz-aware UTC ? (and of course that the schema of the sql table now includes 'with time zone')
>
>But so for databases that don't support timezones. Previously we converted to UTC so users got also back naive UTC, while now they will get back naive local?

Can you answer this one explicitly? Is that last paragraph correct? If so, I think we should mention this in the whatsnew (the behavioural change; the fact itself that you get back naive local is already mentioned in the new io.rst section you added)
Your last paragraph is correct @jorisvandenbossche. I have added that point to the whatsnew section. 
I think the whatsnew note still doesn't reflect the *change* in behaviour; it doesn't mention what it did before (assume you have code that now expects to read back naive utc data (eg you now localize it), but now it will start giving local naive data, which will make the localizing totally wrong). 
And given that it is a rather significant change, I think we should actually add a mention of it to the API changes section (but maybe keep the original one in enhancements section as well). 
@jorisvandenbossche Added a entry to the API changes section noting the change in behavior.
Any additional comments?
ping @jorisvandenbossche 
This one all good @jorisvandenbossche?
Will push this in tomorrow if there are no other comments.
@mroeschke really sorry about the slow reply (I am lagging a bit behind in following up PRs)

I still think we need to make it much more clear what the consequence for users is (and not refer to the user guide which only mentions the current behaviour and not the change in behaviour compared to earlier releases)
Some more concrete feedback:

- let's make it an actual section to give it more weight than just a bullet point in a list
- I would include an example to illustrate it better: if you have a database with a table with timestamp with timezone column, what did it return in 0.23? and what in 0.24?

Such an example will not work in sqlite, so probably need to run it locally with eg postgresql and paste it verbatim in the whatsnew.
@mroeschke unless I am mistaken about the consequence of course .. 
Because I was now trying to write up an example myself, and I am not so sure any more of what I thought would change. 

So above I said the following:

>  >   So if one had now a tz-aware column, it was converted to naive UTC, and you got back naive UTC. So the difference is now that you will get back tz-aware UTC ? (and of course that the schema of the sql table now includes 'with time zone')
> >
> >   But so for databases that don't support timezones. Previously we converted to UTC so users got also back naive UTC, while now they will get back naive local?
>
> Can you answer this one explicitly? Is that last paragraph correct? If so, I think we should mention this in the whatsnew (the behavioural change; the fact itself that you get back naive local is already mentioned in the new io.rst section you added)

on which you answered that this was correct. 

So therefore I assumed there is a significant change in behaviour, which I don't see reflected in the whatsnew note.

But:

- I one has a tz-aware column now, and you try to write it, it actually errors instead of converting to naive UTC as I said above?
- And if one previously read from a database that did support timezones, you already got utc data back, so nothing changed here as well?
So the only significant consequence is described here in the `Datetimelike API Changes`:

https://github.com/pandas-dev/pandas/blob/de6278863571a10b2f35c1e65cd25801a98622aa/doc/source/whatsnew/v0.24.0.txt#L617

Roundtrips Before:
* tz-aware DataFrame --> database that doesn't support timezones --> tz-naive UTC DataFrame
* tz-aware DataFrame --> database that does support timezones --> tz-naive UTC DataFrame

Roundtrips After:
* tz-aware DataFrame --> database that doesn't support timezones --> tz-naive local DataFrame
* tz-aware DataFrame --> database that does support timezones --> tz-aware UTC DataFrame

So addressing your last comment's bullet points:

* No error is raised. The data will now be written as naive local instead of naive UTC
* Correct. Before, pandas read tz aware data from databases as UTC and that behavior has not changed.
Yeah given this discussion; it would be wise to highlight this change as its own section in the whatsnew. 
@jorisvandenbossche added a new whatsnew section in API detailing the before and after implications of this change.
So yesterday I tried to make a small illustrative example showing the changed round-trip, but did it actually work before? 
Because I get:

```
In [1]: import sqlalchemy

In [3]: engine = sqlalchemy.create_engine('sqlite:///:memory:')

In [4]: df = pd.DataFrame({'a':[1,2,3], 'b': pd.date_range('2016-01-01', periods=3, tz='Europe/Brussels')})

In [5]: df.to_sql('table', engine)
...
TypeError: Cannot cast DatetimeIndex to dtype datetime64[us]

In [6]: pd.__version__
Out[6]: '0.23.4'
```

So that is my source of confusion. I thought it worked before (and you seemed to confirm it), so therefore I assumed that there is a *change* in behaviour. But is that actually the case? 
Do you have a running example of the cases that you list in the table above?
Hmm interesting. I made a (bad) assumption that the tests covered this case, but I actually don't see a test case where to_sql is tested with timezones. So the behavior you're seeing is probably correct.

I'll try to confirm tonight on other dbs as well, but I guess it looks like I fixed a bug too :)
I ran the same test on postgres @jorisvandenbossche and I got the same result.

Therefore you're correct in that there really isn't a change in behavior since previously `to_sql` and timezone aware datetimes raised an error. I reflected this in a separate whatsnew entry in the io bug section.
Added a fix for #23510 since it was a pretty easy addition to this PR
Small issue with the new test I think https://travis-ci.org/pandas-dev/pandas/jobs/451739169#L1986

```
_____ TestMySQLAlchemy.test_naive_datetimeindex_roundtrip[load_iris_data0] _____
self = <pandas.tests.io.test_sql.TestMySQLAlchemy object at 0x7febe71173d0>
    def test_naive_datetimeindex_roundtrip(self):
        # GH 23510
        # Ensure that a naive DatetimeIndex isn't converted to UTC
        dates = date_range('2018-01-01', periods=5, freq='6H')
        expected = DataFrame({'nums': range(5)}, index=dates)
        expected.to_sql('foo_table', self.conn, index_label='info_date')
        result = sql.read_sql_table('foo_table', self.conn
                                    index_col='info_date')
>       tm.assert_frame_equal(result, expected)
E       AssertionError: DataFrame.index are different
E       
E       Attribute "names" are different
E       [left]:  [u'info_date']
E       [right]: [None]
```

looks good otherwise.
Thanks @TomAugspurger. Fixed that test (I don't think index name is expected to roundtrip in that test)
Lets's finally merge this! @mroeschke thanks for the endurance :-)