API: Localize Series when calling to_datetime with utc=True (#6415)
State: closed

 - [x] closes #6415
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master -u -- "*.py" | flake8 --diff``
 - [x] whatsnew entry

When `_convert_listlike` passes a `np.array` into a `Series`, the `Series` seems to initially be of naive datetime dtype. Localized `Series` if `utc=True`. 

I made one minor change to an existing test that `tz_localize` and index when it originally set the index with a `Series` with `pd.to_datetime(...utc=True)`.

Labels:
name: API Design
name: Timezones
description: Timezone data dtype

Comments:
I noticed that some IO SQL tests have been failing. 

It appears that in the conversion from SQL to DataFrame the function [`_handle_date_column`](https://github.com/pandas-dev/pandas/blob/master/pandas/io/sql.py#L102) has `pd.to_datetime(..., utc=True)` hardcoded which is now localizing the columns to utc by default after this fix.

I feel we shouldn't assume these database dates are utc by default i.e. remove hardcoded `utc=True`. Thoughts?
@mroeschke yeah, that sql issue might be a tricky issue, as I recall we had a long discussion about that. The `utc=True` was added in https://github.com/pandas-dev/pandas/pull/7364. 
It was done to deal with datetime objects with timezones coming from the database. The end result of using utc=True was that we always converted the timezone aware data to naive utc. 

But, that also dates back from the time that a Series could not hold timezone aware data. So now, I think it would be OK (or even better) to have it as timezone aware but still UTC data (the reason that it has to be utc, if I recall, is that the database uses fixed offsets, which can lead to different timezones around DST, which results in errors when converting that to a datetime64 column. Therefore we decided to always convert timezone aware data to UTC)

So given that, it might be that we just need to update the tests?

actually the db issue might be more complex and just changing tests is not the correct thing to do. See what is actually returned currently in the database.  IIRC postgres stores a datetime-aware tz, but its actually in the *local* timezone, IOW it loses the offset info (it *does* convert to UTC from wherever it was, it just *stores* it locally). SO you need to reverse this on de-serialization.

@cpcloud 
@jreback looking at the issues I linked to (but it is a long time ago I actually looked at it ..), the data we get from postgres are datetime objects with ~pytz~ psycopg2 fixed offsets (so no actual timezone). So I think converting to UTC is, as we decided back then, still the most sensible thing to do (up to now, it only looses the 'utc time zone' as Series back then didn't support having a timezone. So now we can still have a utc data, but just preserve the fact it is timezone aware)


@jorisvandenbossche postgres loses information in the timezone. They *are* UTC dates, but in the local timezone. you need to match the existing behavior.
> you need to match the existing behavior.

Sorry, what do you mean? Do you mean existing behaviour of `read_sql`? 
As I already said, `read_sql` already returns utc data for 'timestamp with time zone' data (so how postgres stores them). It now only converts it to "utc naive data" (as Series could not hold timezone information), which is due to the changes in this PR changes to "utc aware data", so the same values, but now with the timezone information. 
IMO this change is not necessarily bad (I think it would have done that in #7364 if it had been possible at that time)
I agree with @jorisvandenbossche 

For [reference](https://www.postgresql.org/docs/9.1/static/datatype-datetime.html), postgres `timestamp with time zone` is stored in UTC, and upon output: 

>  it is always converted from UTC to the current timezone zone, and displayed as local time in that zone

But more importantly, the psycopg2 driver [returns datetime objects with `psycopg2.tz.FixedOffsetTimezone`](http://initd.org/psycopg/docs/usage.html#time-zones-handling), and since this has historically been converted to UTC (naive), keeping it UTC sounds good to me (but now aware after this PR). I can add an additional note in the whatsnew
Hello @mroeschke! Thanks for updating the PR.











Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on September 01, 2017 at 07:00 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=h1) Report
> Merging [#17109](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/062f6f118fe4ea439ae255a8ff886a532e20ecdb?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #17109      +/-   ##
==========================================
- Coverage   91.03%   91.02%   -0.02%     
==========================================
  Files         163      163              
  Lines       49580    49581       +1     
==========================================
- Hits        45137    45130       -7     
- Misses       4443     4451       +8
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.8% <28.57%> (-0.01%)` | :arrow_down: |
| #single | `40.26% <100%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/tools/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvZGF0ZXRpbWVzLnB5) | `67.3% <100%> (+0.27%)` | :arrow_up: |
| [pandas/io/sql.py](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=tree#diff-cGFuZGFzL2lvL3NxbC5weQ==) | `94.8% <100%> (ø)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.72% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17109?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/17109?src=pr&el=footer). Last update [062f6f1...5eb7d9c](https://codecov.io/gh/pandas-dev/pandas/pull/17109?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@mroeschke Based on the diff in the sql tests, it seems that you now have `read_sql` to return UTC aware data *always* (is this correct?), while IMO we should only return tz aware data when the SQL type is `timestamp with time zone`, and still return plain tz naive datetime data when the SQL type is just `timestamp`
@jorisvandenbossche Ah yes, you're right. I think I was too aggressive in changing the SQL tests. 

I will have to dig deeper into SQL routines to see if I can determine the original SQL type and pass that info along to the [`_handle_date_column`](https://github.com/pandas-dev/pandas/blob/793020293ee1e5fa023f45c12943a4ac51cc23d0/pandas/io/sql.py#L102) function which is now making datetime columns tz aware with `to_datetime(..., utc=True)`
> I will have to dig deeper into SQL routines to see if I can determine the original SQL type and pass that info along to the _handle_date_column function which is now making datetime columns tz aware with to_datetime(..., utc=True)

For the case where we have sql type information, you have to look at `_harmonize_columns`. There is already a step were we handle datetime data, but just do the same thing for both aware and non-aware dtype (the difference is already detected). So there you can easily add a distinction.

For the case where that is not available (`read_sql_query`), I am not sure what currently actually happens.
Thanks for reviewing @jreback and @jorisvandenbossche. Apologies for being idle on this.

@jorisvandenbossche A lot of the SQL tests check the conversion of a `DateCol` (native datetime column), so those tests do not need changes. but I need to double check the tests that convert `DateColWithTz` (tz aware datetime column). 
@jorisvandenbossche There's a singular test, `test_datetime_with_timezone`, for all tz-aware datetime column tests which passed originally. I have added an additional (redundant) test to verify that the columns are being localized to UTC. That specific test were skipping locally, so waiting to see if passes on CI.

P.S. To test the full suite of SQL tests locally, are there additional local configuration steps besides installing `requirements_all.txt`? 
> P.S. To test the full suite of SQL tests locally, are there additional local configuration steps besides installing requirements_all.txt?

I think you need to make a database called 'pandas_nosetest' (and that is accessible for the default postgres user). See here how it tries to connect to the database in the tests:

https://github.com/pandas-dev/pandas/blob/66ec5f3e616f6449ef2c88401042cf2a282234d7/pandas/tests/io/test_sql.py#L1764

and this is the setup on travis:

https://github.com/pandas-dev/pandas/blob/66ec5f3e616f6449ef2c88401042cf2a282234d7/ci/install_db_travis.sh#L10

(but it is a long time ago that I did ran those tests locally)

The SQL tests with datetime tz-aware columns are passing and all green. Let me know if there's anything else I need to address. 
> You didn't have to change anything in the sql tests? That seems a bit suspicious to me (maybe indicating it is actually not working correctly now?)

I looked into the `test_datetime_with_timezone` test, and I think you need to remove the astype in the following lines:

https://github.com/pandas-dev/pandas/blob/f7216efdc5b8f21c903b8c6b425cdedc5e641f36/pandas/tests/io/test_sql.py#L1316-L1320

Due to the astype, it converts the result always to UTC before asserting, so it does not assert it is already UTC as a result from `read_sql_table`.

I assume with the changes that this test will still pass once corrected, but to correctly test the functionality, we need to update that one. You can maybe also add the `assert is_datetime64tz_dtype(col.dtype)` and `assert str(col.dt.tz) == 'UTC'` like a few lines above (to be explicit, although they are actually redundant with the assert_series_equal I think)
Fixed the `test_datetime_with_timezone` test and consolidated the others tests.  Appveyor had connectivity issues, but tests passed on other platforms.
Addressed your comments @jorisvandenbossche and all green. 
Fixed the whatsnew and extraneous comments and created additional tests for tz-aware strings and Series with `datetime[ns]` dtypes.

I also stumbled upon this inconsistency when I was trying to add an additional test for `'datetime[ns, CET]'` dtype:

```
In [3]: pd.Series(['2013-01-01 00:00:00'], dtype='datetime64[ns, CET]')
Out[3]: 
0   2013-01-01 01:00:00+01:00
dtype: datetime64[ns, CET]

In [4]: pd.Series([pd.Timestamp('2013-01-01 00:00:00', tz='CET')])
   ...: 
Out[4]: 
0   2013-01-01 00:00:00+01:00
dtype: datetime64[ns, CET]
```

I think these should be consistent and `Out[4]` should be the correct behavior. Thoughts? If so, I can add a new issue and maybe a test in this PR that will xfail due to this. 
I think all outstanding comments were addressed now, so time to finally get this merged :-)

> I also stumbled upon this inconsistency when I was trying to add an additional test for 'datetime[ns, CET]' dtype:

I think Out[4] is certainly correct, for the `Out[3]:` I am not really sure. 
(but I would certainly keep this for a separate issue / PR)