[SPARK-31005][SQL] Support time zone ids in casting strings to timestamps by MaxGekk · Pull Request #27753 · apache/spark

MaxGekk · 2020-03-01T18:36:33Z

What changes were proposed in this pull request?

In the PR, I propose to change DateTimeUtils.stringToTimestamp to support any valid time zone id at the end of input string. After the changes, the function accepts zone ids in the formats:

no zone id. In that case, the function uses the local session time zone from the SQL config spark.sql.session.timeZone
-[h]h:[m]m
+[h]h:[m]m
Z
Short zone id, see https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html#SHORT_IDS
Zone ID starts with 'UTC+', 'UTC-', 'GMT+', 'GMT-', 'UT+' or 'UT-'. The ID is split in two, with a two or three letter prefix and a suffix starting with the sign. The suffix must be in the formats:
- +|-h[h]
- +|-hh[:]mm
- +|-hh:mm:ss
- +|-hhmmss
Region-based zone IDs in the form {area}/{city}, such as Europe/Paris or America/New_York. The default set of region ids is supplied by the IANA Time Zone Database (TZDB).

Why are the changes needed?

To use stringToTimestamp as a substitution of removed stringToTime, see [SPARK-30960][SQL] add back the legacy date/timestamp format support in CSV/JSON parser #27710 (comment)
Improve UX of Spark SQL by allowing flexible formats of zone ids. Currently, Spark accepts only Z and zone offsets that can be inconvenient when a time zone offset is shifted due to daylight saving rules. For instance:

spark-sql> select cast('2015-03-18T12:03:17.123456 Europe/Moscow' as timestamp);
NULL

Does this PR introduce any user-facing change?

Yes. After the changes, casting strings to timestamps allows time zone id at the end of the strings:

spark-sql> select cast('2015-03-18T12:03:17.123456 Europe/Moscow' as timestamp);
2015-03-18 12:03:17.123456

How was this patch tested?

Added new test cases to the string to timestamp test in DateTimeUtilsSuite.
Run CastSuite and AnsiCastSuite.

SparkQA · 2020-03-01T22:37:58Z

Test build #119143 has finished for PR 27753 at commit f107331.

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

MaxGekk · 2020-03-02T08:00:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

          }
        } else if (i == 5 || i == 6) {
-          if (b == 'Z') {
+          if (b == '-' || b == '+') {


getZoneId() is able to handle zone offsets w/ prefix - and + but it doesn't support the format 7:3 like in

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

Line 164 in ffc0935

checkCastStringToTimestamp("2015-03-18T12:03:17+7:3", new Timestamp(c.getTimeInMillis))

. So, I have to keep the code for backward compatibility.

MaxGekk · 2020-03-02T08:02:05Z

@cloud-fan @HyukjinKwon Please, review the PR. I haven't updated comments for stringToTimestamp yet. I will do that when I will address your comment later.

SparkQA · 2020-03-03T17:01:58Z

Test build #119228 has finished for PR 27753 at commit bdfe5f3.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

cloud-fan · 2020-03-04T10:27:25Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

      checkStringToTimestamp("2015-03-18T12:03:17.123121+7:30", expected)
-
-      zoneId = getZoneId("GMT+07:30")
-      expected = Option(date(2015, 3, 18, 12, 3, 17, 123120, zid = zoneId))


why drop this test? 123120 is different from 123121

reverted it back, and added more tests

cloud-fan · 2020-03-04T14:51:35Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala

            i += 1
-            tz = Some(43)
-          } else if (b == '-' || b == '+') {
+            tz = Some(new String(bytes, j, 1))


why not just b.toChar.toString?

Just for consistency with another change

SparkQA · 2020-03-04T20:00:01Z

Test build #119309 has finished for PR 27753 at commit 00a07ab.

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

cloud-fan · 2020-03-05T12:49:57Z

thanks, merging to master/3.0!

…amps ### What changes were proposed in this pull request? In the PR, I propose to change `DateTimeUtils.stringToTimestamp` to support any valid time zone id at the end of input string. After the changes, the function accepts zone ids in the formats: - no zone id. In that case, the function uses the local session time zone from the SQL config `spark.sql.session.timeZone` - -[h]h:[m]m - +[h]h:[m]m - Z - Short zone id, see https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html#SHORT_IDS - Zone ID starts with 'UTC+', 'UTC-', 'GMT+', 'GMT-', 'UT+' or 'UT-'. The ID is split in two, with a two or three letter prefix and a suffix starting with the sign. The suffix must be in the formats: - +|-h[h] - +|-hh[:]mm - +|-hh:mm:ss - +|-hhmmss - Region-based zone IDs in the form `{area}/{city}`, such as `Europe/Paris` or `America/New_York`. The default set of region ids is supplied by the IANA Time Zone Database (TZDB). ### Why are the changes needed? - To use `stringToTimestamp` as a substitution of removed `stringToTime`, see #27710 (comment) - Improve UX of Spark SQL by allowing flexible formats of zone ids. Currently, Spark accepts only `Z` and zone offsets that can be inconvenient when a time zone offset is shifted due to daylight saving rules. For instance: ```sql spark-sql> select cast('2015-03-18T12:03:17.123456 Europe/Moscow' as timestamp); NULL ``` ### Does this PR introduce any user-facing change? Yes. After the changes, casting strings to timestamps allows time zone id at the end of the strings: ```sql spark-sql> select cast('2015-03-18T12:03:17.123456 Europe/Moscow' as timestamp); 2015-03-18 12:03:17.123456 ``` ### How was this patch tested? - Added new test cases to the `string to timestamp` test in `DateTimeUtilsSuite`. - Run `CastSuite` and `AnsiCastSuite`. Closes #27753 from MaxGekk/stringToTimestamp-uni-zoneId. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 1fd9a91) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…amps ### What changes were proposed in this pull request? In the PR, I propose to change `DateTimeUtils.stringToTimestamp` to support any valid time zone id at the end of input string. After the changes, the function accepts zone ids in the formats: - no zone id. In that case, the function uses the local session time zone from the SQL config `spark.sql.session.timeZone` - -[h]h:[m]m - +[h]h:[m]m - Z - Short zone id, see https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html#SHORT_IDS - Zone ID starts with 'UTC+', 'UTC-', 'GMT+', 'GMT-', 'UT+' or 'UT-'. The ID is split in two, with a two or three letter prefix and a suffix starting with the sign. The suffix must be in the formats: - +|-h[h] - +|-hh[:]mm - +|-hh:mm:ss - +|-hhmmss - Region-based zone IDs in the form `{area}/{city}`, such as `Europe/Paris` or `America/New_York`. The default set of region ids is supplied by the IANA Time Zone Database (TZDB). ### Why are the changes needed? - To use `stringToTimestamp` as a substitution of removed `stringToTime`, see apache#27710 (comment) - Improve UX of Spark SQL by allowing flexible formats of zone ids. Currently, Spark accepts only `Z` and zone offsets that can be inconvenient when a time zone offset is shifted due to daylight saving rules. For instance: ```sql spark-sql> select cast('2015-03-18T12:03:17.123456 Europe/Moscow' as timestamp); NULL ``` ### Does this PR introduce any user-facing change? Yes. After the changes, casting strings to timestamps allows time zone id at the end of the strings: ```sql spark-sql> select cast('2015-03-18T12:03:17.123456 Europe/Moscow' as timestamp); 2015-03-18 12:03:17.123456 ``` ### How was this patch tested? - Added new test cases to the `string to timestamp` test in `DateTimeUtilsSuite`. - Run `CastSuite` and `AnsiCastSuite`. Closes apache#27753 from MaxGekk/stringToTimestamp-uni-zoneId. Authored-by: Maxim Gekk <max.gekk@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

MaxGekk added 2 commits March 1, 2020 21:34

Add tests

40b4e6b

Support time zone ids in stringToTimestamp

f107331

MaxGekk mentioned this pull request Mar 1, 2020

[SPARK-30960][SQL] add back the legacy date/timestamp format support in CSV/JSON parser #27710

Closed

MaxGekk changed the title ~~[WIP][SQL] Support time zone ids in casting strings to timestamps~~ [SPARK-31005][SQL] Support time zone ids in casting strings to timestamps Mar 2, 2020

MaxGekk commented Mar 2, 2020

View reviewed changes

Update the comment for stringToTimestamp

bdfe5f3

cloud-fan reviewed Mar 4, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala Show resolved Hide resolved

cloud-fan reviewed Mar 4, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Show resolved Hide resolved

cloud-fan reviewed Mar 4, 2020

View reviewed changes

Add more tests

00a07ab

cloud-fan reviewed Mar 4, 2020

View reviewed changes

cloud-fan approved these changes Mar 4, 2020

View reviewed changes

cloud-fan closed this in 1fd9a91 Mar 5, 2020

MaxGekk deleted the stringToTimestamp-uni-zoneId branch June 5, 2020 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-31005][SQL] Support time zone ids in casting strings to timestamps#27753

[SPARK-31005][SQL] Support time zone ids in casting strings to timestamps#27753
MaxGekk wants to merge 4 commits intoapache:masterfrom
MaxGekk:stringToTimestamp-uni-zoneId

MaxGekk commented Mar 1, 2020 •

edited

Loading

Uh oh!

SparkQA commented Mar 1, 2020

Uh oh!

MaxGekk Mar 2, 2020

Uh oh!

MaxGekk commented Mar 2, 2020

Uh oh!

SparkQA commented Mar 3, 2020

Uh oh!

Uh oh!

Uh oh!

cloud-fan Mar 4, 2020

Uh oh!

MaxGekk Mar 4, 2020

Uh oh!

cloud-fan Mar 4, 2020

Uh oh!

MaxGekk Mar 4, 2020

Uh oh!

SparkQA commented Mar 4, 2020

Uh oh!

cloud-fan commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

MaxGekk commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Mar 1, 2020

Uh oh!

MaxGekk Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Mar 2, 2020

Uh oh!

SparkQA commented Mar 3, 2020

Uh oh!

Uh oh!

Uh oh!

cloud-fan Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fan Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 4, 2020

Uh oh!

cloud-fan commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MaxGekk commented Mar 1, 2020 •

edited

Loading