[SPARK-32272][SQL] Add SQL standard command SET TIME ZONE #29064

yaooqinn · 2020-07-10T11:21:33Z

What changes were proposed in this pull request?

This PR adds the SQL standard command - SET TIME ZONE to the current default time zone displacement for the current SQL-session, which is the same as the existing `set spark.sql.session.timeZone=xxx'.

All in all, this PR adds syntax as following,

SET TIME ZONE LOCAL;
SET TIME ZONE 'valid time zone';  -- zone offset or region
SET TIME ZONE INTERVAL XXXX; -- xxx must in [-18, + 18] hours, * this range is bigger than ansi  [-14, + 14]

Why are the changes needed?

ANSI compliance and supply pure SQL users a way to retrieve all supported TimeZones

Does this PR introduce any user-facing change?

yes, add new syntax.

How was this patch tested?

add unit tests.

and locally verified reference doc

yaooqinn · 2020-07-10T11:23:53Z

cc @cloud-fan @maropu @dongjoon-hyun thanks for reviewing

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ListTimeZonesCommand.scala

cloud-fan · 2020-07-10T12:31:11Z

because it has to be qualified with interval qualifier HOUR TO MINUTE which is not fully supported in Spark now.

HOUR TO MINUTE is a supported interval literal syntax, isn't it?

yaooqinn · 2020-07-10T13:19:20Z

because it has to be qualified with interval qualifier HOUR TO MINUTE which is not fully supported in Spark now.

HOUR TO MINUTE is a supported interval literal syntax, isn't it?

OK, I will add fully support then.

SparkQA · 2020-07-10T14:26:42Z

Test build #125602 has finished for PR 29064 at commit 13056b5.

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

SparkQA · 2020-07-10T14:28:54Z

Test build #125604 has finished for PR 29064 at commit 225695f.

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

yaooqinn · 2020-07-13T04:02:38Z

the test case for listing all timezones is removed because it varies from different JDKs.

The interval is supported and also some extensions were made too

offset range is [-18, +18] in spark which is bigger than ansi's,
the second part is supported in Spark, e.g. +14:14:14, while ansi only indicates hours-minutes,
multi value-units interval is much common than unit-to-unit ones, so this interval form is supported too.

cc @cloud-fan

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

SparkQA · 2020-07-13T06:52:03Z

Test build #125749 has finished for PR 29064 at commit 3a62ecc.

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

cloud-fan · 2020-07-13T09:29:30Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

+      val interval = parseIntervalLiteral(ctx.interval)
+      if (interval.months != 0 || interval.days != 0 ||
+        math.abs(interval.microseconds) > 18 * DateTimeConstants.MICROS_PER_HOUR) {
+        throw new ParseException("The interval value must be in the range of [-18, +18] hours",


shall we also fail for things like INTERVAL 1 MICROSECOND?

If we want to fail this like this completely, we need a new interval parser, otherwise, we can only forbid it based on the interval result. e.g. INTERVAL 1 MICROSECOND -1 MICROSECOND 1 HOUR will work...

INTERVAL 1 MICROSECOND -1 MICROSECOND 1 HOUR is OK. We should forbid truncation like INTERVAL 1 MICROSECOND, which you have to ignore the 1 microsecond.

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

cloud-fan · 2020-07-13T09:31:11Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+    | SET TIME ZONE ALL                                                #listTimeZones
+    | SET TIME ZONE interval                                           #setTimeZone
+    | SET TIME ZONE timezone=(STRING | LOCAL)                          #setTimeZone
+    | SET TIME ZONE .*?                                                #setTimeZone


so we add this only for better parser message?

this is used to fail invalid set time zone syntax explicitly, cuz' now we support

spark-sql (default)> set time zone abcd; key value time zone abcd <undefined>

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

cloud-fan · 2020-07-13T09:32:46Z

sql/core/src/test/resources/sql-tests/results/timezone.sql.out

+-- !query schema
+struct<key:string,value:string>
+-- !query output
+spark.sql.session.timeZone	Asia/Hong_Kong


This looks like a weird output for SET TIME ZONE 'Asia/Hong_Kong'. Shall we add a Project node on the SetCommand to give better output?

What do we expect as output? only keep the value Asia/Hong_Kong here?

is there any reference?

postgres=# SET TIME ZONE 'Asia/Hong_kong'; SET

SparkQA · 2020-07-13T14:10:45Z

Test build #125759 has finished for PR 29064 at commit 95a756a.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-13T14:55:19Z

Test build #125763 has finished for PR 29064 at commit c95fa7d.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-13T19:47:51Z

Test build #125773 has finished for PR 29064 at commit 5501213.

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

maropu · 2020-07-13T23:34:56Z

retest this please

SparkQA · 2020-07-14T04:40:25Z

Test build #125791 has finished for PR 29064 at commit 5501213.

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

cloud-fan · 2020-07-14T08:59:35Z

We should also add a document page in SQL reference for it.

yaooqinn · 2020-07-15T06:18:06Z

cc @maropu @cloud-fan @huaxingao. Please check the reference doc for set tz command.

cloud-fan · 2020-07-15T16:51:10Z

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

+
+### Description
+
+The `SET TIME ZONE` command sets the current default time zone(`spark.sql.session.timeZone`) displacement for the `SparkSession`. 


The SET TIME ZONE command sets the time zone of the current session.

cloud-fan · 2020-07-15T16:52:12Z

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

+
+* **LOCAL**
+
+    Respectively set the time zone the one specified in environment variable `TZ`, or to the operating system time zone if `TZ` is undefined.


set the time zone "to" the one specified

operating system -> system? We actually use the JVM timezone which can be different from the operating system,

yeah, it prefers user.timezone

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

SparkQA · 2020-07-15T20:23:29Z

Test build #125891 has finished for PR 29064 at commit b3ac6f4.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

huaxingao · 2020-07-15T20:00:15Z

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

+
+* **LOCAL**
+
+    Respectively set the time zone the one specified in environment variable `TZ`, or to the operating system time zone if `TZ` is undefined.


nit: Respectively set -> Sets?

huaxingao · 2020-07-15T21:31:00Z

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

+
+* **interval_literal**
+
+    The [interval literal](sql-ref-literals.html#interval-literal) represents the displacement of time zone to the 'UTC'. It must be in the range of [-18, 18] hours and max to second precision, e.g. `INTERVAL 2 HOURS 30 MINITUES` or `INTERVAL '15:40:32' HOUR TO SECOND`.


I guess something like this?
The interval literal represents the difference between the (system?) time zone and UTC.

huaxingao · 2020-07-15T21:31:05Z

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

+SET TIME ZONE LOCAL;
+
+-- Set time zone to the region-based zone ID.
+SET TIME ZONE 'America/Los_Angeles'


nit: ; in the end?

huaxingao · 2020-07-15T21:37:17Z

@yaooqinn Please also change menu-sql.yaml to add SET TIME ZONE to the side menu.

SparkQA · 2020-07-16T07:05:02Z

Test build #125940 has finished for PR 29064 at commit e5aa3b3.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-16T08:35:38Z

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md

+
+* **LOCAL**
+
+    Set the time zone to the one specified in the java `user.timezone` property or environment variable `TZ`, or to the system time zone if they are undefined.


can we clarify what happens if both the java user.timezone property or environment variable TZ are specified?

How about

Set the time zone to the one specified in the java `user.timezone` property, or to the environment variable `TZ` if `user.timezone` is undefined, or to the system time zone if both of them are undefined.

SparkQA · 2020-07-16T12:30:08Z

Test build #125959 has finished for PR 29064 at commit 11f0fed.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-07-16T12:46:49Z

retest this please

cloud-fan · 2020-07-16T13:00:56Z

All github action checks passed, I think we are good to go. Thanks, merging to master!

SparkQA · 2020-07-16T16:08:31Z

Test build #125963 has finished for PR 29064 at commit a4c60f3.

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

SparkQA · 2020-07-16T18:57:11Z

Test build #125974 has finished for PR 29064 at commit a4c60f3.

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

gatorsmile · 2020-08-16T03:57:40Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

@@ -240,6 +240,9 @@ statement
    | MSCK REPAIR TABLE multipartIdentifier                            #repairTable
    | op=(ADD | LIST) identifier (STRING | .*?)                        #manageResource
    | SET ROLE .*?                                                     #failNativeCommand
+    | SET TIME ZONE interval                                           #setTimeZone
+    | SET TIME ZONE timezone=(STRING | LOCAL)                          #setTimeZone
+    | SET TIME ZONE .*?                                                #setTimeZone


It sounds like none of the systems are following ANSI SQL syntax completely.

https://www.ibm.com/support/knowledgecenter/SSEPEK_10.0.0/sqlref/src/tpc/db2z_sql_setsessiontimezone.html

https://www.postgresql.org/docs/12/sql-set.html

https://dev.mysql.com/doc/refman/8.0/en/time-zone-support.html

https://docs.oracle.com/cd/E11882_01/server.112/e10729/ch4datetime.htm#NLSPG263

We are close to the DB2 syntax, except that we support interval and LOCAL, and we don't allow the optional SESSION keyword.

[SPARK-32272][SQL] Add and extend SQL standard command SET TIME ZONE

7910c9a

probot-autolabeler bot added the SQL label Jul 10, 2020

add more tests

13056b5

fix parser

225695f

MaxGekk requested changes Jul 10, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ListTimeZonesCommand.scala Outdated Show resolved Hide resolved

support interval

3a62ecc

cloud-fan reviewed Jul 13, 2020

View reviewed changes

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4 Outdated Show resolved Hide resolved

yaooqinn added 3 commits July 13, 2020 15:49

add tests

ef1f3ed

add doc

95a756a

rm list all api

c95fa7d

probot-autolabeler bot added the DOCS label Jul 13, 2020

cloud-fan reviewed Jul 13, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Show resolved Hide resolved

cloud-fan reviewed Jul 13, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jul 13, 2020

View reviewed changes

yaooqinn changed the title ~~[SPARK-32272][SQL] Add and extend SQL standard command SET TIME ZONE~~ [SPARK-32272][SQL] Add SQL standard command SET TIME ZONE Jul 13, 2020

check second precision

5501213

cloud-fan approved these changes Jul 14, 2020

View reviewed changes

doc

b3ac6f4

cloud-fan reviewed Jul 15, 2020

View reviewed changes

docs/sql-ref-syntax-aux-conf-mgmt-set-timezone.md Show resolved Hide resolved

cloud-fan reviewed Jul 15, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala Show resolved Hide resolved

huaxingao reviewed Jul 15, 2020

View reviewed changes

address comments and fix tests

e5aa3b3

Merge remote-tracking branch 'ssh/master' into SPARK-32272

11f0fed

cloud-fan reviewed Jul 16, 2020

View reviewed changes

doc

a4c60f3

cloud-fan approved these changes Jul 16, 2020

View reviewed changes

cloud-fan closed this in bdeb626 Jul 16, 2020

gatorsmile reviewed Aug 16, 2020

View reviewed changes


		### Description

		The `SET TIME ZONE` command sets the current default time zone(`spark.sql.session.timeZone`) displacement for the `SparkSession`.


		* LOCAL

		Respectively set the time zone the one specified in environment variable `TZ`, or to the operating system time zone if `TZ` is undefined.


		* interval_literal

		The [interval literal](sql-ref-literals.html#interval-literal) represents the displacement of time zone to the 'UTC'. It must be in the range of [-18, 18] hours and max to second precision, e.g. `INTERVAL 2 HOURS 30 MINITUES` or `INTERVAL '15:40:32' HOUR TO SECOND`.


		* LOCAL

		Set the time zone to the one specified in the java `user.timezone` property or environment variable `TZ`, or to the system time zone if they are undefined.

[SPARK-32272][SQL] Add SQL standard command SET TIME ZONE #29064

[SPARK-32272][SQL] Add SQL standard command SET TIME ZONE #29064

Conversation

yaooqinn commented Jul 10, 2020 • edited

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?

yaooqinn commented Jul 10, 2020

cloud-fan commented Jul 10, 2020

yaooqinn commented Jul 10, 2020

SparkQA commented Jul 10, 2020

SparkQA commented Jul 10, 2020

yaooqinn commented Jul 13, 2020

SparkQA commented Jul 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Jul 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 13, 2020

SparkQA commented Jul 13, 2020

SparkQA commented Jul 13, 2020

maropu commented Jul 13, 2020

SparkQA commented Jul 14, 2020

cloud-fan commented Jul 14, 2020

yaooqinn commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaxingao commented Jul 15, 2020

SparkQA commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 16, 2020

yaooqinn commented Jul 16, 2020

cloud-fan commented Jul 16, 2020

SparkQA commented Jul 16, 2020

SparkQA commented Jul 16, 2020

gatorsmile Aug 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Jul 10, 2020 •

edited

cloud-fan Jul 13, 2020 •

edited

gatorsmile Aug 16, 2020 •

edited