Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump pandas to 0.25.3 #8985

Merged
merged 2 commits into from
Jan 22, 2020
Merged

Bump pandas to 0.25.3 #8985

merged 2 commits into from
Jan 22, 2020

Conversation

villebro
Copy link
Member

@villebro villebro commented Jan 18, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

On the "What’s new in 1.0.0 (??)" page https://dev.pandas.io/docs/whatsnew/v1.0.0.html , the following is stated: "It is recommended to first upgrade to pandas 0.25 and to ensure your code is working without warnings, before upgrading to pandas 1.0." As Superset is still on 0.24, it makes sense to bump to 0.25 now, as 1.0 is currently nearing release.

Bumping required a small change in superset.result_set.py due to Pandas now being more explicit about timezones. The new logic in 0.25 caused the original datetimes to be shifted by the UTC offset, as they were interpreted as being UTC due to the missing tzinfo. I traced back the change to this PR: pandas-dev/pandas#25263

@codecov-io
Copy link

Codecov Report

Merging #8985 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8985   +/-   ##
=======================================
  Coverage   59.16%   59.16%           
=======================================
  Files         367      367           
  Lines       11679    11679           
  Branches     2862     2862           
=======================================
  Hits         6910     6910           
  Misses       4590     4590           
  Partials      179      179

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fc5fd4...21c30f7. Read the comment docs.

@villebro villebro changed the title [WIP] Bump pandas to 0.25.3 to get ready for 1.0 Bump pandas to 0.25.3 Jan 19, 2020
@villebro
Copy link
Member Author

@robdiciuccio , as this touches code that you recently changed, it would be great if you could take a look at this PR.

@robdiciuccio
Copy link
Member

robdiciuccio commented Jan 20, 2020

@villebro getting the following error when loading data from a timestamptz column in postgres:

ERROR:root:Already tz-aware, use tz_convert to convert.
Traceback (most recent call last):
  File "/Users/rob/work/incubator-superset/superset/result_set.py", line 96, in __init__
    series = pd.to_datetime(series).dt.tz_localize(tz)
  File "/Users/rob/work/incubator-superset/venv/lib/python3.6/site-packages/pandas/core/accessor.py", line 91, in f
    return self._delegate_method(name, *args, **kwargs)
  File "/Users/rob/work/incubator-superset/venv/lib/python3.6/site-packages/pandas/core/indexes/accessors.py", line 94, in _delegate_method
    result = method(*args, **kwargs)
  File "/Users/rob/work/incubator-superset/venv/lib/python3.6/site-packages/pandas/core/accessor.py", line 91, in f
    return self._delegate_method(name, *args, **kwargs)
  File "/Users/rob/work/incubator-superset/venv/lib/python3.6/site-packages/pandas/core/indexes/datetimelike.py", line 721, in _delegate_method
    result = operator.methodcaller(name, *args, **kwargs)(self._data)
  File "/Users/rob/work/incubator-superset/venv/lib/python3.6/site-packages/pandas/core/arrays/datetimes.py", line 1049, in tz_localize
    raise TypeError("Already tz-aware, use tz_convert to convert.")
TypeError: Already tz-aware, use tz_convert to convert.

EDIT: scratch that, newer version of Pandas was not loaded 🤦‍♂

Copy link
Member

@robdiciuccio robdiciuccio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested against Postgres timestamp with time zone columns.

@villebro
Copy link
Member Author

Thanks for testing @robdiciuccio . I also verified that this works with both timestamp and timestamptz on Postgres and a few other dbs, so I believe this is good to go.

@villebro
Copy link
Member Author

I'm going to go ahead and merge this, and I haven't been able to turn up any regressions through regular use. If this does surface regressions, I will give priority to fix the errors + unit tests.

@villebro villebro merged commit e46ff23 into apache:master Jan 22, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants