[SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function #34896

yoda-mon · 2021-12-14T13:06:57Z

What changes were proposed in this pull request?

Support 2-arguments by the convert_timezone() function.
If users omit sourceTz, spark takes it from sourceTimestamp or SESSION_LOCAL_TIMEZONE.

Why are the changes needed?

Users can call this API with fewer arguments. The same function in Snowflake supports 2 arguments.

Does this PR introduce any user-facing change?

Yes, users can call this function with 2 arguments.

How was this patch tested?

unit tests

resolve ga error

SparkQA · 2021-12-14T14:00:20Z

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

SparkQA · 2021-12-14T14:55:38Z

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

SparkQA · 2021-12-14T18:08:48Z

Test build #146184 has finished for PR 34896 at commit a802c64.

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

MaxGekk

Please, make ConvertTimezone as a time zone aware expression.

MaxGekk · 2021-12-15T08:19:04Z

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

@@ -3011,13 +3011,17 @@ object SubtractDates {
  arguments = """
    Arguments:
      * sourceTz - the time zone for the input timestamp
+          - if not specified, SESSION_LOCAL_TIMEZONE is used when sourceTs is timestamp_ntz,


SESSION_LOCAL_TIMEZONE is not valid for end users. Point out the SQL config spark.sql.session.timeZone or better SESSION_LOCAL_TIMEZONE.key

MaxGekk · 2021-12-15T08:21:40Z

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

@@ -3028,6 +3032,15 @@ case class ConvertTimezone(
    sourceTs: Expression)
  extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {

+  def this(targetTz: Expression, sourceTs: Expression) = {
+    this(
+      Literal(SQLConf.get.sessionLocalTimeZone),


Please, make the expression as time zone aware via extending TimeZoneAwareExpression.

MaxGekk · 2021-12-15T08:30:24Z

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

+      Literal(SQLConf.get.sessionLocalTimeZone),
+      targetTz,
+      if (sourceTs.dataType == TimestampNTZType) sourceTs
+      else Cast(sourceTs, TimestampNTZType)


How other types can appear if this below

override def inputTypes: Seq[AbstractDataType] = Seq(StringType, StringType, TimestampNTZType)

accepts only TimestampNTZType. Could add an end-to-end tests to timestamp-ltz.sql like
https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/timestamp-ntz.sql#L19

AmplabJenkins · 2021-12-16T03:22:36Z

Can one of the admins verify this patch?

yoda-mon · 2021-12-16T10:35:44Z

@MaxGekk Let me confirm how to extends TimeZoneAwareExpression.
At first I simply extends basic constructor which takes 3 arguments and overides timeZoneId and withTimeZone
https://github.com/yoda-mon/spark/pull/2/files
It seems to work fine and passed all tests, but I wonder if you want me to extends the constructor which takes 2 arguments, or to use withTimeZone in the constructor.
I took auxiliary constructor for case class, so I think it is difficult to implement both two ways. Should I switch to use companion object ?

MaxGekk · 2021-12-27T10:13:11Z

@yoda-mon

Extend TimeZoneAwareExpression and add one more arg timeZoneId to the case class

case class ConvertTimezone(
    sourceTz: Expression,
    targetTz: Expression,
    sourceTs: Expression,
    timeZoneId: Option[String] = None)
  extends TernaryExpression
  with ImplicitCastInputTypes
  with NullIntolerant
  with TimeZoneAwareExpression {

Override withTimeZone()

  override def withTimeZone(timeZoneId: String): TimeZoneAwareExpression = {
    copy(timeZoneId = Option(timeZoneId))
  }

Add 2 constructors that take 2 and 3 arguments

  def this(sourceTz: Expression, targetTz: Expression, sourceTs: Expression) =
    this(sourceTz, targetTz, sourceTs, None)
  def this(targetTz: Expression, sourceTs: Expression) =
    this(Literal("UTC", StringType), targetTz, sourceTs)

The source time zone in the second constructor doesn't matter since you are not going to use it.

yoda-mon · 2022-01-07T07:40:15Z

@MaxGekk
Sorry for the delayed response, I tried to follow your advice.
However at

  def this(targetTz: Expression, sourceTs: Expression) =
    this(Literal("UTC"), targetTz, sourceTs)

sourceTz is not ignored. After the constructor I could use timeZoneId assourceTz that copied by the mix-in, but in that case, it's difficult to distinguish TimeStampLTZ and TimeStampNTZ.

Please let me know if there is something I missed to use the mix-in.

yoda-mon · 2022-01-17T10:15:55Z

@MaxGekk
Gentle reminder,

Extending TimeZoneAwareExpression seems not so simple.
- sourceTz is not ignored so I have to set appropriate timezone there.
- At the constructor, timeZoneId by mix-in could not be used because it would be copied after that point.
- At nullSafeEval or doGenCode I could use timeZoneId, but I have to check whether sourceTs is TimeStampNTZ or TimeStampLTZ so I have to pass it in some ways.

I think there is small benefit for mixing TimeZoneAwareExpression... Is the first idea is simple enough to add the option ?

cloud-fan · 2022-03-03T07:14:04Z

@MaxGekk

…tion Add new constructor to the `ConvertTimezone` expression (see apache#34817) which accepts only 2 arguments: 1. `<targetTz>` - the time zone to which the input timestamp should be converted. 2. `<sourceTs>` - the timestamp to convert. and sets `<sourceTz>` to the current session time zone (see the SQL config `spark.sql.session.timeZone`). Closes apache#34896 To help users in migrations from other systems to Spark SQL. Other systems support optional first parameter: - https://docs.aws.amazon.com/redshift/latest/dg/CONVERT_TIMEZONE.html - https://docs.snowflake.com/en/sql-reference/functions/convert_timezone.html No, it extends the existing signature, and the function hasn't been released yet. By running new tests: ``` $ build/sbt "sql/test:testOnly org.apache.spark.sql.expressions.ExpressionInfoSuite" $ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z timestamp-ltz.sql" $ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z timestamp-ntz.sql" ``` Closes apache#35951 from MaxGekk/convert_timezone-2-params. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 8eb8a42) Signed-off-by: Max Gekk <max.gekk@gmail.com>

yoda-mon added 7 commits December 13, 2021 21:25

Implement convert_timezone with 2 arguments

61869cf

Implement simple testcase

e671124

fix indentation

cbda188

trigger GA

5f484aa

ommit ntz example

f43b7e6

test ga

8b45cf4

Merge pull request #1 from yoda-mon/checktest

a802c64

resolve ga error

github-actions bot added the SQL label Dec 14, 2021

HyukjinKwon requested a review from MaxGekk December 15, 2021 00:41

HyukjinKwon changed the title ~~[SPARK-37568][SQL]Support 2-arguments by the convert_timezone() function~~ [SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function Dec 15, 2021

MaxGekk requested changes Dec 15, 2021

View reviewed changes

MaxGekk mentioned this pull request Mar 23, 2022

[SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function #35951

Closed

MaxGekk closed this in 8eb8a42 Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function #34896

[SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function #34896

yoda-mon commented Dec 14, 2021

SparkQA commented Dec 14, 2021

SparkQA commented Dec 14, 2021

SparkQA commented Dec 14, 2021

MaxGekk left a comment

MaxGekk Dec 15, 2021

MaxGekk Dec 15, 2021

MaxGekk Dec 15, 2021

AmplabJenkins commented Dec 16, 2021

yoda-mon commented Dec 16, 2021

MaxGekk commented Dec 27, 2021

yoda-mon commented Jan 7, 2022

yoda-mon commented Jan 17, 2022

cloud-fan commented Mar 3, 2022

[SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function #34896

[SPARK-37568][SQL] Support 2-arguments by the convert_timezone() function #34896

Conversation

yoda-mon commented Dec 14, 2021

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?

SparkQA commented Dec 14, 2021

SparkQA commented Dec 14, 2021

SparkQA commented Dec 14, 2021

MaxGekk left a comment

Choose a reason for hiding this comment

MaxGekk Dec 15, 2021

Choose a reason for hiding this comment

MaxGekk Dec 15, 2021

Choose a reason for hiding this comment

MaxGekk Dec 15, 2021

Choose a reason for hiding this comment

AmplabJenkins commented Dec 16, 2021

yoda-mon commented Dec 16, 2021

MaxGekk commented Dec 27, 2021

yoda-mon commented Jan 7, 2022

yoda-mon commented Jan 17, 2022

cloud-fan commented Mar 3, 2022