-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
ARROW-6652: [Python] Fix ChunkedArray.to_pandas to retain timezone #5471
ARROW-6652: [Python] Fix ChunkedArray.to_pandas to retain timezone #5471
Conversation
python/pyarrow/table.pxi
Outdated
if tz is not None: | ||
tz = string_to_tzinfo(tz) | ||
result = (result.dt.tz_localize('utc') | ||
.dt.tz_convert(tz)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure those 6 lines are worth a separate helper function (now this is duplicated from Array.to_pandas), but if others prefer I can certainly do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's small but carries domain knowledge, so I'd rather see a helper function (perhaps in the pandas_compat
module).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I made a small helper function (see new push).
For now I still put it behind the if check to only need to import it from pandas_compat if it would actually be needed. But that made that it is not much shorter as before in lines of code .. If find this clearer in the code to read, but I can also always call the helper function and move the if check inside the helper function.
Codecov Report
@@ Coverage Diff @@
## master #5471 +/- ##
===========================================
- Coverage 88.64% 66.39% -22.25%
===========================================
Files 958 505 -453
Lines 127522 69887 -57635
Branches 1498 0 -1498
===========================================
- Hits 113039 46403 -66636
- Misses 14118 23484 +9366
+ Partials 365 0 -365
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @jorisvandenbossche ! I confirmed that with this change and #5465 I can pass Spark integration tests locally.
dbf788a
to
89d0044
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Follow-up on #5462 to also apply this fix for ChunkedArray.