You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/27 08:55:13 UTC
[GitHub] [spark] MaxGekk opened a new pull request, #39239: [WIP][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
MaxGekk opened a new pull request, #39239:
URL: https://github.com/apache/spark/pull/39239
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058176658
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
> it was a naive datetime that is a local time when it's collected before
Just coping of an offset from the epoch in UTC and saying that it is a local offset is incorrect, I do believe. So, the current impl is wrong definitely. What I did here and semantically the same what we do while converting TimestampType to external Java type - java.time.Instant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1465148669
@HyukjinKwon @cloud-fan A problem of PySpark's timestamp_ltz is it is a local timestamp, and not a physical timestamp that timestamp_ltz is supposed to be. Let's see even Java 7 timestamp (it also has some issues but it is still a physical time point not local timestamp):
```
➜ ~ TZ=America/Los_Angeles ./spark-3.3/bin/spark-shell
Welcome to
____ __
/ __/__ ___ _____/ /__
_\ \/ _ \/ _ `/ __/ '_/
/___/ .__/\_,_/_/ /_/\_\ version 3.3.2
/_/
scala> spark.conf.set("spark.sql.session.timeZone", "Europe/Moscow")
scala> val df = sql("select timestamp'1970-01-01T00:00:00+0000'")
df: org.apache.spark.sql.DataFrame = [TIMESTAMP '1970-01-01 03:00:00': timestamp]
scala> df.collect()(0).getTimestamp(0).toGMTString
res1: String = 1 Jan 1970 00:00:00 GMT
```
The timestamp still knows how to show itself in the UTC time zone because it contains an offset from the epoch, and can render itself in any time zone.
Let's see at PySpark timestamp:
```
$ TZ=America/Los_Angeles ./spark-3.3/bin/pyspark
Welcome to
____ __
/ __/__ ___ _____/ /__
_\ \/ _ \/ _ `/ __/ '_/
/__ / .__/\_,_/_/ /_/\_\ version 3.3.2
/_/
>>> spark.conf.set("spark.sql.session.timeZone", "Europe/Moscow")
>>>
>>> df = sql("select timestamp'1970-01-01T00:00:00+0000'")
>>> df.collect()[0][0].utctimetuple()
time.struct_time(tm_year=1969, tm_mon=12, tm_mday=31, tm_hour=16, tm_min=0, tm_sec=0, tm_wday=2, tm_yday=365, tm_isdst=0)
```
Since python's timestamp becomes a local timestamp at the conversion:
```python
return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
```
it cannot render itself in the UTC time zone correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058433158
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
> TimestampNTZType is a UTC time.
This is NO time zone timestamp that means local or logical timestamp. For example, 2022-12-28 20:23:31 could be in UTC, America/Los_Angeles and Europe/Amsterdam. The same local timestamp represents 3 different physical timestamps. So, this type matches precisely to
```python
datetime.datetime(year, month, day, hour, minute, second, microsecond, tzinfo=None)
```
> My understanding was that TimestampType is a local time per Spark's session timezone ...
Yep, this one must be bound to a time zone:
```python
datetime.datetime(year, month, day, hour, minute, second, microsecond, tzinfo=ZoneInfo('US/Pacific'))
```
or
```python
datetime.datetime(year, month, day, hour, minute, second, microsecond, tzinfo=datetime.timezone.utc)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1138306144
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
+ .replace(microsecond=ts % 1000000)
+ # Convert to the local time zone and remove the time zone info to
+ # to have the result type `datetime64[us]` for compatibility.
+ .astimezone(None)
+ .replace(tzinfo=None)
Review Comment:
shall we remove timezone for timestamp ntz as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
URL: https://github.com/apache/spark/pull/39239
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058787040
##########
python/pyspark/pandas/tests/test_resample.py:
##########
@@ -263,7 +263,7 @@ def test_dataframe_resample(self):
def test_series_resample(self):
self._test_resample(self.pdf1.A, self.psdf1.A, ["4Y"], ["sum"])
self._test_resample(self.pdf2.A, self.psdf2.A, ["13M"], ["sum"])
- self._test_resample(self.pdf3.A, self.psdf3.A, ["18H"], ["sum"])
+ # self._test_resample(self.pdf3.A, self.psdf3.A, ["18H"], ["sum"])
Review Comment:
does this test still fail?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1756825518
@MaxGekk we gave it up because it breaks some tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058142903
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
Hmmm ... I believe this isn't technically correct ... it was a naive datetime that is a local time when it's collected before. Now they are datetimes are in UTC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1366021084
@HyukjinKwon @itholic All 4 failed tests are related to Pandas, and it seems Pandas code suffers from the issue too but I cannot find where the conversion happens. Could you point me out the place, please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1366342520
Let me take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058277577
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
I think there're implementation details that we should fix. My take was (see quotes from https://docs.python.org/3/library/datetime.html#aware-and-naive-objects)
> ... Whether a naive object represents Coordinated Universal Time (UTC), local time, or time in some other timezone is purely up to the program ...
With `TimestampType`, naive datetimes are interpreted as a local time. Meaning that `java.time.LocalDateTime`
With `TimestampNTZType`, naive datetimes are interpreted as a UTC time. Meaning that `java.time.Instant`
But reading that codes, SQL side is opposite. My understanding was that `TimestampType` is a local time per Spark's session timezone, and `TimestampNTZType` is a UTC time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058794838
##########
python/pyspark/pandas/tests/test_resample.py:
##########
@@ -263,7 +263,7 @@ def test_dataframe_resample(self):
def test_series_resample(self):
self._test_resample(self.pdf1.A, self.psdf1.A, ["4Y"], ["sum"])
self._test_resample(self.pdf2.A, self.psdf2.A, ["13M"], ["sum"])
- self._test_resample(self.pdf3.A, self.psdf3.A, ["18H"], ["sum"])
+ # self._test_resample(self.pdf3.A, self.psdf3.A, ["18H"], ["sum"])
Review Comment:
It fails locally on my laptop. The test itself is weird, what's the idea of sum up all timestamps? If you need to compare timestamp series, just compare elements. Summing of dates/timestamps doesn't make any sense. Any way, so far, it fails. It seems somewhere in pandas code there is a dependency from time zone offset, or conversion from/to local timestamp is not symmetric (actually the conversion is not needed but we are doing it for some reason).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058786637
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
Review Comment:
We need domain experts to introduce the semantic of python datetime.
For the timestamp (ltz) type, the semantic is: it's a physical time point, and can map to different wall clock time with different local timezone. In Java, the best match is `Instant`, but I'm not sure about python `datetime` though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1469834155
Okay, I got the problem now. The fix seems fine but we should probably fix `toInternal` too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] keen85 commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "keen85 (via GitHub)" <gi...@apache.org>.
keen85 commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1648051067
@MaxGekk & @cloud-fan thanks for working on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1138428968
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
+ .replace(microsecond=ts % 1000000)
+ # Convert to the local time zone and remove the time zone info to
+ # to have the result type `datetime64[us]` for compatibility.
+ .astimezone(None)
+ .replace(tzinfo=None)
Review Comment:
Timestamp ntz should be fine. Scala Java also shift them to UTC if I read the codepath correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1469830006
Alright. Python's `datetime` is up to the program to deal with naive datetimes (https://docs.python.org/3/library/datetime.html#aware-and-naive-objects):
> Whether a naive object represents Coordinated Universal Time (UTC), local time, or time in some other timezone is purely up to the program
So if we argue that datetime with Timestanp NTZ is a datetime without timezone. ` df.collect()[0][0].utctimetuple()` has to be ` df.collect()[0][0].timetuple()`.
So the problem here would be implementation detail. Previously, we get the physical timestamp assuming it's a local time. Now we should get the physical timestamp assuming the time is in UTC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1137571293
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
+ .replace(microsecond=ts % 1000000)
+ # Convert to the local time zone and remove the time zone info to
+ # to have the result type `datetime64[us]` for compatibility.
+ .astimezone(None)
+ .replace(tzinfo=None)
Review Comment:
Let me check this, and add a roundtrip test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1471268820
Yup, I meant that most of cases work except few cases that can happen because of timezone. we're on the same page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1138553272
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
+ .replace(microsecond=ts % 1000000)
+ # Convert to the local time zone and remove the time zone info to
+ # to have the result type `datetime64[us]` for compatibility.
+ .astimezone(None)
+ .replace(tzinfo=None)
Review Comment:
yea Java side also shift the timestamp to UTC but its value is timezone-less (`LocalDateTime`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058520899
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
@HyukjinKwon As possible workaround, the result could be converted to local time zone, and the attached time zone could be removed. For instance:
```python
datetime.datetime
# Set the time zone to UTC because the TIMESTAMP type stores timestamps
# as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
# in the UTC time zone.
.fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
microsecond=ts % 1000000
).astimezone(None).replace(tzinfo=None)
```
but this `ts` should be interpreted as an offset in UTC time zone from the epoch otherwise it is incorrect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058143100
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
One workaround would be to use TimestampNTZType instead of doing this I guess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] pboulos commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "pboulos (via GitHub)" <gi...@apache.org>.
pboulos commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1084211356
##########
python/pyspark/pandas/tests/test_resample.py:
##########
@@ -263,7 +263,7 @@ def test_dataframe_resample(self):
def test_series_resample(self):
self._test_resample(self.pdf1.A, self.psdf1.A, ["4Y"], ["sum"])
self._test_resample(self.pdf2.A, self.psdf2.A, ["13M"], ["sum"])
- self._test_resample(self.pdf3.A, self.psdf3.A, ["18H"], ["sum"])
+ # self._test_resample(self.pdf3.A, self.psdf3.A, ["18H"], ["sum"])
Review Comment:
Is there any movement on this? I'm the one who raised this internally and I've been referred to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1136904292
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
+ .replace(microsecond=ts % 1000000)
+ # Convert to the local time zone and remove the time zone info to
+ # to have the result type `datetime64[us]` for compatibility.
+ .astimezone(None)
+ .replace(tzinfo=None)
Review Comment:
We should probably fix `toInternal` too. If this shifts from UTC to a local time, `toInternal` should shifts the local time to UTC. Otherwise, roundtrip wouldn't work properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1058683926
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,15 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc).replace(
Review Comment:
^ SGTM. At least, we should keep returning naive datetimes ....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1138430073
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
+ .replace(microsecond=ts % 1000000)
+ # Convert to the local time zone and remove the time zone info to
+ # to have the result type `datetime64[us]` for compatibility.
+ .astimezone(None)
+ .replace(tzinfo=None)
Review Comment:
Here what we need for ltz is set the timezones as utc but don't shitf the offset for the zone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1476075130
LGTM if all tests pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #39239:
URL: https://github.com/apache/spark/pull/39239#discussion_r1138305065
##########
python/pyspark/sql/types.py:
##########
@@ -276,7 +276,18 @@ def toInternal(self, dt: datetime.datetime) -> int:
def fromInternal(self, ts: int) -> datetime.datetime:
if ts is not None:
# using int to avoid precision loss in float
- return datetime.datetime.fromtimestamp(ts // 1000000).replace(microsecond=ts % 1000000)
+ return (
+ datetime.datetime
+ # Set the time zone to UTC because the TIMESTAMP type stores timestamps
+ # as the number of microseconds from the epoch of 1970-01-01T00:00:00.000000Z
+ # in the UTC time zone.
+ .fromtimestamp(ts // 1000000, tz=datetime.timezone.utc)
Review Comment:
use `utcfromtimestamp`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #39239: [SPARK-41730][PYTHON] Set tz to UTC while converting of timestamps to python's datetime
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #39239:
URL: https://github.com/apache/spark/pull/39239#issuecomment-1470569694
> So the problem here would be implementation detail.
@HyukjinKwon I think this is not impl details but a fundamental problem of `datetime`, especially in the corner case of `0001-01-01 00:00:00+00:00`. Since the last one is a physical timestamp in the `+00:00` zone offset, it can be mapped to **local** timestamps >= `0001-01-01 00:00:00` as well as to timestamps < `0001-01-01 00:00:00`, for instance `0001-01-01 16:00:00 B.C.` ( the local date/time before the common (or current) era). And the last one is not acceptable by python's `datetime`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org