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.

Sign up for GitHub

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

Jump to bottom

[SPARK-23360][SQL][PYTHON] Get local timezone from environment via pytz, or dateutil. #20559

Closed
wants to merge 6 commits into from
Closed

[SPARK-23360][SQL][PYTHON] Get local timezone from environment via pytz, or dateutil. #20559

wants to merge 6 commits into from

Conversation

ueshin
Copy link
Member

@ueshin ueshin commented Feb 9, 2018 •
edited
Loading

What changes were proposed in this pull request?

Currently we use tzlocal() to get Python local timezone, but it sometimes causes unexpected behavior.
I changed the way to get Python local timezone to use pytz if the timezone is specified in environment variable, or timezone file via dateutil .

How was this patch tested?

Added a test and existing tests.

@ueshin
Copy link
Member Author

ueshin commented Feb 9, 2018

cc @icexelloss @BryanCutler @felixcheung @HyukjinKwon @cloud-fan

@ueshin ueshin changed the title [WIP][SPARK-23360][SQL][PYTHON] Get local timezone from environment vi pytz, or dateutil. [WIP][SPARK-23360][SQL][PYTHON] Get local timezone from environment via pytz, or dateutil. Feb 9, 2018
@SparkQA
Copy link

SparkQA commented Feb 9, 2018

Test build #87260 has finished for PR 20559 at commit e87bd76.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

icexelloss
icexelloss reviewed Feb 9, 2018
View reviewed changes
python/pyspark/sql/types.py Outdated
def _get_local_timezone():
""" Get local timezone from environment vi pytz, or dateutil. """
from pyspark.sql.utils import require_minimum_pandas_version
require_minimum_pandas_version()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it isn't needed here. I'll remove it. Thanks.

icexelloss
icexelloss reviewed Feb 9, 2018
View reviewed changes
python/pyspark/sql/types.py Outdated
require_minimum_pandas_version()

import os
return os.environ.get('TZ', 'dateutil/:')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand how does "dateutil/:" work, can you maybe add some comments for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add some comments.

@icexelloss
Copy link
Contributor

Thanks @ueshin for the quick patch!

BryanCutler
BryanCutler reviewed Feb 9, 2018
View reviewed changes
Copy link
Member

@BryanCutler BryanCutler left a comment

Choose a reason for hiding this comment

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

Thanks @ueshin for the fix! Do we know why tzlocal() was inconsistent - is it because of DST and ambiguous flag?

python/pyspark/sql/tests.py Outdated
@@ -4124,7 +4126,7 @@ def test_vectorized_udf_timestamps(self):
data = [(0, datetime(1969, 1, 1, 1, 1, 1)),
(1, datetime(2012, 2, 2, 2, 2, 2)),
(2, None),
(3, datetime(2100, 3, 3, 3, 3, 3))]
(3, datetime(2100, 4, 4, 4, 4, 4))]
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if changing these values made a difference somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll revert it.

python/pyspark/sql/types.py Outdated
@@ -1709,6 +1709,15 @@ def _check_dataframe_convert_date(pdf, schema):
return pdf


def _get_local_timezone():
""" Get local timezone from environment vi pytz, or dateutil. """
Copy link
Member

Choose a reason for hiding this comment

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

did you mean "from environment via pytz"? the 'TZ' environment var is read by the datetime module, does pytz do anything with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I modified and added the comment. Can you understand by the comment?

@ueshin
Copy link
Member Author

ueshin commented Feb 10, 2018 •
edited
Loading

@BryanCutler Seems like pandas handles tzlocal() differently than other timezones, and it might handle DST incorrectly, I guess.

E.g.,

@SparkQA
Copy link

SparkQA commented Feb 10, 2018

Test build #87278 has finished for PR 20559 at commit e20e9fd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ueshin ueshin changed the title [WIP][SPARK-23360][SQL][PYTHON] Get local timezone from environment via pytz, or dateutil. [SPARK-23360][SQL][PYTHON] Get local timezone from environment via pytz, or dateutil. Feb 10, 2018
HyukjinKwon
HyukjinKwon reviewed Feb 10, 2018
View reviewed changes
python/pyspark/sql/types.py Outdated
# `s.dt.tz_localize('tzlocal()')` doesn't work properly when including NaT.
return s.apply(lambda ts: ts.tz_localize(from_tz).tz_convert(to_tz).tz_localize(None)
if ts is not pd.NaT else pd.NaT)
return s.dt.tz_localize(from_tz).dt.tz_convert(to_tz).dt.tz_localize(None)
Copy link
Member

Choose a reason for hiding this comment

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

@ueshin, is it safe to remove if ts is not pd.NaT else pd.NaT? Seems there is a small possibility for tzlocal():

https://github.com/pandas-dev/pandas/blob/0.19.x/pandas/tslib.pyx#L1760
https://github.com/pandas-dev/pandas/blob/0.19.x/pandas/tslib.pyx#L54
https://github.com/dateutil/dateutil/blob/2.6.1/dateutil/tz/tz.py#L1362
https://github.com/dateutil/dateutil/blob/2.6.1/dateutil/tz/tz.py#L1408

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I'll revert this.

HyukjinKwon
HyukjinKwon reviewed Feb 10, 2018
View reviewed changes
python/pyspark/sql/tests.py Outdated
self.spark.conf.set('spark.sql.session.timeZone', tz)

df = self.spark.createDataFrame(pdf)
df.show()
Copy link
Member

Choose a reason for hiding this comment

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

gentle reminder for it. Seems it's now there for debugging purpose I guess? :).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I should've removed it. Thanks!

HyukjinKwon
HyukjinKwon approved these changes Feb 10, 2018
View reviewed changes
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM except a question and a nit.

@SparkQA
Copy link

SparkQA commented Feb 10, 2018

Test build #87286 has finished for PR 20559 at commit a082e8c.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Feb 10, 2018

Test build #87287 has finished for PR 20559 at commit a082e8c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request Feb 10, 2018
@ueshin @HyukjinKwon
[SPARK-23360][SQL][PYTHON] Get local timezone from environment via py…
b7571b9 
…tz, or dateutil.

## What changes were proposed in this pull request?

Currently we use `tzlocal()` to get Python local timezone, but it sometimes causes unexpected behavior.
I changed the way to get Python local timezone to use pytz if the timezone is specified in environment variable, or timezone file via dateutil .

## How was this patch tested?

Added a test and existing tests.

Author: Takuya UESHIN <[email protected]>

Closes #20559 from ueshin/issues/SPARK-23360/master.

(cherry picked from commit 97a224a)
Signed-off-by: hyukjinkwon <[email protected]>
@HyukjinKwon
Copy link
Member

Merged to master and branch-2.3.

@asfgit asfgit closed this in 97a224a Feb 10, 2018
robert3005 pushed a commit to palantir/spark that referenced this pull request Feb 12, 2018
@ueshin
[SPARK-23360][SQL][PYTHON] Get local timezone from environment via py…
c857333 
…tz, or dateutil.

## What changes were proposed in this pull request?

Currently we use `tzlocal()` to get Python local timezone, but it sometimes causes unexpected behavior.
I changed the way to get Python local timezone to use pytz if the timezone is specified in environment variable, or timezone file via dateutil .

## How was this patch tested?

Added a test and existing tests.

Author: Takuya UESHIN <[email protected]>

Closes apache#20559 from ueshin/issues/SPARK-23360/master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icexelloss icexelloss icexelloss left review comments

@BryanCutler BryanCutler BryanCutler left review comments

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ueshin @SparkQA @icexelloss @HyukjinKwon @BryanCutler