[SPARK-33469][SQL] Add current_timezone function #30400

ulysses-you · 2020-11-17T17:37:01Z

What changes were proposed in this pull request?

Add a CurrentTimeZone function and replace the value at Optimizer side.

Why are the changes needed?

Let user get current timezone easily. Then user can call

SELECT current_timezone()

Presto: https://prestodb.io/docs/current/functions/datetime.html
SQL Server: https://docs.microsoft.com/en-us/sql/t-sql/functions/current-timezone-transact-sql?view=sql-server-ver15

Does this PR introduce any user-facing change?

Yes, a new function.

How was this patch tested?

Add test.

SparkQA · 2020-11-17T18:20:34Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35832/

SparkQA · 2020-11-17T18:30:51Z

Test build #131229 has finished for PR 30400 at commit 2e48507.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class CurrentTimeZone() extends LeafExpression with Unevaluable

SparkQA · 2020-11-17T18:50:23Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35832/

dongjoon-hyun

Thank you, @ulysses-you . Is the function name current_timezone popular in the other DBMSs?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

ulysses-you · 2020-11-18T04:22:43Z

@dongjoon-hyun yes, presto has this function.

SparkQA · 2020-11-18T05:15:24Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35850/

SparkQA · 2020-11-18T05:39:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35850/

SparkQA · 2020-11-18T08:05:02Z

Test build #131247 has finished for PR 30400 at commit 73f71c5.

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

dongjoon-hyun · 2020-11-20T18:04:03Z

Retest this please

dongjoon-hyun · 2020-11-20T18:05:45Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

@@ -73,6 +73,21 @@ trait TimestampFormatterHelper extends TimeZoneAwareExpression {
  }
 }

+@ExpressionDescription(
+  usage = "_FUNC_() - Returns the current timezone.",


Do we need to say that it's a session local timezone explicitly?

dongjoon-hyun · 2020-11-20T18:07:35Z

sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

+  test("SPARK-33469: Add current_timezone function") {
+    val df = Seq(1).toDF("c")
+    val timezone = df.selectExpr("current_timezone()").collect().head.getString(0)
+    assert(timezone == SQLConf.get.sessionLocalTimeZone)


For this test, is it possible to set sessionLocalTimeZone explicitly and check with it?

dongjoon-hyun · 2020-11-20T18:08:40Z

sql/core/src/test/scala/org/apache/spark/sql/expressions/ExpressionInfoSuite.scala

@@ -150,6 +150,7 @@ class ExpressionInfoSuite extends SparkFunSuite with SharedSparkSession {
      "org.apache.spark.sql.catalyst.expressions.CurrentDate",
      "org.apache.spark.sql.catalyst.expressions.CurrentTimestamp",
      "org.apache.spark.sql.catalyst.expressions.Now",
+      "org.apache.spark.sql.catalyst.expressions.CurrentTimeZone",


Shall we add this after CurrentTimestamp like the following?

"org.apache.spark.sql.catalyst.expressions.CurrentTimestamp", "org.apache.spark.sql.catalyst.expressions.CurrentTimeZone", "org.apache.spark.sql.catalyst.expressions.Now",

dongjoon-hyun

cc @cloud-fan and @MaxGekk

SparkQA · 2020-11-20T18:51:26Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36041/

MaxGekk

I worry about the motivations for adding the new function to public API.

Let user get current timezone easily.

At the moment, users can get the session time zone from the SQL config spark.sql.session.timeZone. It doesn't seem to be so difficult, doesn't it?

Presto, SQL Server ...

As far as I know Spark doesn't declare to be compatible to those systems.

What is the real use case when the function is preferable over reading the SQL config?

dongjoon-hyun · 2020-11-20T19:12:29Z

Good point, @MaxGekk . Could you give us your use cases, @ulysses-you ?

IMO, in the Spark SQL world (Spark Thrift Server or Spark SQL Shell), the following is the only way.

spark-sql> set spark.sql.session.timeZone;
spark.sql.session.timeZone	America/Los_Angeles
Time taken: 0.034 seconds, Fetched 1 row(s)

In other words, we cannot use it in Spark SQL DML statements. I guess you have some use cases for that.

SparkQA · 2020-11-20T19:25:20Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36041/

SparkQA · 2020-11-20T23:04:44Z

Test build #131435 has finished for PR 30400 at commit 73f71c5.

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

ulysses-you · 2020-11-21T01:37:30Z

@dongjoon-hyun @MaxGekk The difference between config and function, I think the later can calculate with other data. That says we can do some compare or save the data into storage engine. We introduce many date-related function in recent version, then the timezone is important.

In other words, we cannot use it in Spark SQL DML statements. I guess you have some use cases for that.

ah, actually I have not seen some case with it and just consider we may need it sometime.

We can also show the offset as @maropu said that make it different with set spark.sql.session.timeZone.

SparkQA · 2020-11-21T02:22:42Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36062/

SparkQA · 2020-11-21T02:52:07Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36062/

dongjoon-hyun

Please let me and @ulysses-you know your final opinion, @maropu and @MaxGekk .
Although this looks good enough for me, I'll leave this PR open for your opinions. Please feel free to comments any final opinions.

Also, cc @cloud-fan .

SparkQA · 2020-11-21T04:38:23Z

Test build #131456 has finished for PR 30400 at commit b591ff7.

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

dongjoon-hyun · 2020-11-21T16:28:27Z

Could you rebase this, @ulysses-you ? There was a bug fix for GitHub Action Hive test failure in master branch.

maropu · 2020-11-21T23:54:55Z

Since this feature might be useful for SQL users, adding it looks fine to me.

…3469

SparkQA · 2020-11-22T06:58:51Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36100/

SparkQA · 2020-11-22T07:30:19Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36100/

SparkQA · 2020-11-22T08:05:02Z

Test build #131496 has finished for PR 30400 at commit c83da46.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class ExecutorSource(
case class GetShufflePushMergerLocations(numMergersNeeded: Int, hostsToFilter: Set[String])
case class RemoveShufflePushMergerLocation(host: String) extends ToBlockManagerMaster
abstract class LikeAllBase extends UnaryExpression with ImplicitCastInputTypes with NullIntolerant
case class LikeAll(child: Expression, patterns: Seq[UTF8String]) extends LikeAllBase
case class NotLikeAll(child: Expression, patterns: Seq[UTF8String]) extends LikeAllBase
case class ParseUrl(children: Seq[Expression], failOnError: Boolean = SQLConf.get.ansiEnabled)
implicit class MetadataColumnsHelper(metadata: Array[MetadataColumn])

dongjoon-hyun · 2020-11-22T23:36:31Z

Thank you, @ulysses-you and @maropu and @MaxGekk .
Merged to master for Apache Spark 3.1.0.

ulysses-you · 2020-11-23T07:22:23Z

thanks for merging !

init

2e48507

github-actions bot added the SQL label Nov 17, 2020

dongjoon-hyun reviewed Nov 17, 2020

View reviewed changes

maropu reviewed Nov 17, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Show resolved Hide resolved

maropu reviewed Nov 17, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Show resolved Hide resolved

example ingore

82b65b9

golden file

73f71c5

dongjoon-hyun reviewed Nov 20, 2020

View reviewed changes

MaxGekk reviewed Nov 20, 2020

View reviewed changes

address comment

b591ff7

dongjoon-hyun approved these changes Nov 21, 2020

View reviewed changes

maropu approved these changes Nov 21, 2020

View reviewed changes

Merge branch 'master' of https://github.com/apache/spark into SPARK-3…

c83da46

…3469

dongjoon-hyun closed this in 6d625cc Nov 22, 2020

ulysses-you deleted the SPARK-33469 branch November 22, 2021 10:26

[SPARK-33469][SQL] Add current_timezone function #30400

[SPARK-33469][SQL] Add current_timezone function #30400

Uh oh!

Conversation

ulysses-you commented Nov 17, 2020 • edited by wangyum 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 Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

SparkQA commented Nov 17, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ulysses-you commented Nov 18, 2020

Uh oh!

SparkQA commented Nov 18, 2020

Uh oh!

SparkQA commented Nov 18, 2020

Uh oh!

SparkQA commented Nov 18, 2020

Uh oh!

dongjoon-hyun commented Nov 20, 2020

Uh oh!

dongjoon-hyun Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Nov 20, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

SparkQA commented Nov 20, 2020

Uh oh!

ulysses-you commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 21, 2020

Uh oh!

SparkQA commented Nov 21, 2020

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 21, 2020

Uh oh!

dongjoon-hyun commented Nov 21, 2020

Uh oh!

maropu commented Nov 21, 2020

Uh oh!

SparkQA commented Nov 22, 2020

Uh oh!

SparkQA commented Nov 22, 2020

Uh oh!

SparkQA commented Nov 22, 2020

Uh oh!

dongjoon-hyun commented Nov 22, 2020

ulysses-you commented Nov 17, 2020 •

edited by wangyum

Loading

dongjoon-hyun commented Nov 20, 2020 •

edited

Loading

ulysses-you commented Nov 21, 2020 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading