BUG: Can't restore index from parquet with offset-specified timezone #35997

[alippai]

• closes BUG: Reading from parquet throws UnknownTimeZoneError using timezone-aware date in index #35997
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @alippai! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-07 00:09:08 UTC

[dsaxton]

Thanks @alippai, however the test doesn't seem to be passing? I think we would first need to fix the issue before committing the test.

[alippai]

Like with TDD, you write the expectation as a testcase and it fails, then the code is changed and the untouched test passes. Isn't this the workflow for fixing a bug? I didn't work on pandas codebase before, but I can give it a try.

[alippai]

BTW Setting up the environment works like a charm, but the thing I did is in the official docs too: https://pandas.pydata.org/docs/dev/development/contributing.html#test-driven-development-code-writing

So, before actually writing any code, you should write your tests. Often the test can be taken from the original GitHub issue.

[dsaxton]

It's a good practice to write tests first, but a PR should include both the test and the fix that makes the test pass (unless the test is already passing of course)

[alippai]

@dsaxton @jbrockmendel I've pushed a naive fix, what do you think? Should I skip regex or is it OK?

[dsaxton]

Where else is maybe_get_tz used? Curious why this wouldn't have broken anything previously. Also I think this change likely requires more tests in /tests/tslibs/test_timezones.py.

[jreback]

can you run asv's associated with timezones and see how this performs.

[alippai]

@jreback Run it, there are no significant changes within 10% (beside the fact that index_cached_properties.IndexCache.* looks totally random)

[alippai]

I'm giving this up for now, feel free to grab & fix it

[jorisvandenbossche]

Regardless of the arrow/parquet bug (which I also fixed in pyarrow now), I think this change is probably good, because it basically means that we accept the string repr of datetime.timezone fixed offsets.

Adding it to the fixture ensures that it is tested in a variety of tests, but are we sure there is somewhere a test that specifically asserts the correctness of the conversion (the code added in maybe_get_tz)? I would maybe add an explicit test for this (not relying on that fixture).

[jorisvandenbossche]

Can you also add it as a column (can use the same values) ?

[alippai]

Nice catch, actually this assertion fails:

AssertionError: Attributes of DataFrame.iloc[:, 0] (column name="index_as_col") are different

Attribute "dtype" are different
[left]:  datetime64[ns, UTC-02:15]
[right]: datetime64[ns, pytz.FixedOffset(-135)]

[alippai]

@jorisvandenbossche now that pyarrow does something unexpected I'm not sure how to proceed with this PR. Can you advice, please?

[jorisvandenbossche]

It's not necessarily unexpected from pyarrow (it just always uses pytz (for better or worse) for fixed offsets, it only stores the actual fixed offset, not what class was used on the python side, but will answer about that on the Arrow PR). So I would just test the current behaviour: you can create the expected dataframe (with the different timezone) and pass this to check_round_trip.

Separately from this, I am wondering if we should treat fixed offset datetime.timezone(..) and pytz.FixedOffset(..) as equal when comparing dtypes (@jbrockmendel ?). Or at least have an utility or keyword in the assert_ to indicate they can be considered equal.

[alippai]

@jorisvandenbossche I exposed the check_dtype parameter for now, this way it passes. What do you think?

[jreback]

can you change this so that on future versions of pyarrow it will set check_dtype=True (IOW when the bug is fixed). otherwise I am worried this will perpetuate forever.

alternatively, could remove the check_dtype and just xfail this (and when new pyarrow fixes this test will then start to xpass which will make it fail as we have strict=True). so again could make this a conditional on an older / newer version of pyarrow.

[alippai]

@jreback based on the comment above by @jorisvandenbossche , I don't expect this to be "fixed" in Arrow. pytz.FixedOffset is a wrapper around timedelta like datetime.timezone is and they both implement datetime.tzinfo. ofc I'm happy to do any change you ask

[jorisvandenbossche]

Yes, there are no active plans in pyarrow to change this. Although it makes probably sense to use datetime.timezone, but if we do that, it's pytz.FixedOffset that won't be preserved, in which case another test probably needs a check_dtype=False.

[alippai]

@jreback I added a long description. I know this won't help a follow up improvement in the future, but at this point it's unlikely to change.

There are two scenarios:

1. arrow/parquet persists the initial class information
2. the assert_ introduces a sub-feature of check_dtype=True (check_timezone_dtype=False by default) accepting pytz.FixedOffset and datetime.timezone equality

I don't see any of them on the short or mid-term roadmap.

[alippai]

Do you need any changes? What else needed for the merge?

[jreback]

Do you need any changes? What else needed for the merge?

ok this lgtm. if you'd add a whatsnew note in 1.2 under I/O section can get this in . ping on green.

[alippai]

@jreback The what's new entry is added, the CI failures are not related. Thanks!

[jreback]

sorry can u in particular call out parquet roundtrioping here as this is where this is likely to affect t the user

[alippai]

You are right, how about now?

[jreback]

thanks for hanging in there @alippai nice work!

[alippai]

This was my first contribution to pandas, thank you @dsaxton, @jbrockmendel, @jorisvandenbossche and @jreback for the help. I really enjoyed working on this and I appreciate the fact that I had close to zero technical issues setting up the dev environment.