[SPARK-31797][SQL] Adds TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS functions #28534

TJX2014 · 2020-05-15T01:22:05Z

What changes were proposed in this pull request?

Add and register three new functions: TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS
A test is added.

Why are the changes needed?

People will have convenient way to get timestamps from seconds,milliseconds and microseconds.

Does this PR introduce any user-facing change?

Yes, people will have the following ways to get timestamp:

sql("select TIMESTAMP_SECONDS(t.a) as timestamp from values(1230219000),(-1230219000) as t(a)").show(false)

+-------------------------+
|timestamp                  |
+-------------------------+
|2008-12-25 23:30:00|
|1931-01-07 16:30:00|
+-------------------------+

sql("select TIMESTAMP_MILLIS(t.a) as timestamp from values(1230219000123),(-1230219000123) as t(a)").show(false)

+-------------------------------+
|timestamp                           |
+-------------------------------+
|2008-12-25 23:30:00.123|
|1931-01-07 16:29:59.877|
+-------------------------------+

sql("select TIMESTAMP_MICROS(t.a) as timestamp from values(1230219000123123),(-1230219000123123) as t(a)").show(false)

+------------------------------------+
|timestamp                                   |
+------------------------------------+
|2008-12-25 23:30:00.123123|
|1931-01-07 16:29:59.876877|
+------------------------------------+

How was this patch tested?

Unit test.

bart-samwel · 2020-05-15T08:30:37Z

This is a behavior change for existing Spark workloads. Even if the code is not compatible with Hive here, we shouldn't break existing workloads. See also the versioning policy.. If we really want to fix this, we would have to introduce a compatibility flag, and fail this construct by default so that people can choose. A better solution would be to introduce functions that are explicit about the meaning of their source operand. E.g. this is how it is done in BigQuery, with functions like TIMESTAMP_MILLIS to get a timestamp from a Unix miliseconds timestamp.

@cloud-fan @MaxGekk FYI

TJX2014 · 2020-05-15T08:41:04Z

@bart-samwel Nice suggestion, I am both ok in conform to BigQuery or Hive.

cloud-fan · 2020-05-15T09:18:34Z

This is not standard behavior. It's OK to follow Hive, but this behavior has been released a long time ago, and is the behavior of Spark for many years. It's too late to change now.

Agree with @bart-samwel to add a dedicated function to convert milli/micro seconds to timestamp.

TJX2014 · 2020-05-15T09:28:51Z

@cloud-fan @bart-samwel Thanks, I will make a revert and to add a dedicated function to support it.

TJX2014 · 2020-05-16T12:13:13Z

@cloud-fan @MaxGekk Could you please help me review it ?

cloud-fan · 2020-05-17T02:34:38Z

ok to test

SparkQA · 2020-05-17T07:05:02Z

Test build #122749 has finished for PR 28534 at commit 48e81cc.

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

SparkQA · 2020-05-18T06:35:42Z

Test build #122765 has finished for PR 28534 at commit 7d562f4.

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

TJX2014 · 2020-05-18T07:28:44Z

Test build #122765 has finished for PR 28534 at commit 7d562f4.

This patch passes all tests.

This patch merges cleanly.

This patch adds no public classes.

@cloud-fan Could you please verify this patch

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

bart-samwel · 2020-05-18T09:19:49Z

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

+    withSQLConf() {
+      checkEvaluation(
+        GetTimestamp(
+          Literal(1580184371847000L),


Test with literals that don't end in three 0s?

Ok, I will replace three 0s with 123 in order to avoid mistake

bart-samwel · 2020-05-18T09:20:18Z

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

+          Literal("milli")), 1580184371847000000L)
+      checkEvaluation(
+        GetTimestamp(
+          Literal(1580184371847000L),


Also test with negative (pre-Unix epoch) timestamps.

You are right, I will add negative test.

bart-samwel · 2020-05-18T09:20:39Z

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

+      checkExceptionInExpression[IllegalArgumentException](
+        GetTimestamp(
+          Literal(1580184371847000L),
+          Literal("other")),


Also test with a non-literal input. The error should still make sense then.

@bart-samwel Could you please give me an example about what a non-literal means.

bart-samwel · 2020-05-18T09:22:28Z

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

@@ -846,6 +853,8 @@ abstract class ToTimestamp
              case NonFatal(_) => null
            }
          }
+        case LongType =>
+          Math.multiplyExact(t.asInstanceOf[Long], scaleFactor.asInstanceOf[Long])


@cloud-fan Do we do range checks on timestamps? (Do we even have published valid ranges?) Because if we do, then this could produce valid Long values that are out of range.

I will add a check when decide the value of scaleFactor to avoid the result of multiply out of range of Long value.

SparkQA · 2020-05-18T09:42:46Z

Test build #122799 has finished for PR 28534 at commit 019d6b2.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

TJX2014

correct some issue base on suggestions

TJX2014 · 2020-05-18T09:44:17Z

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

+    withSQLConf() {
+      checkEvaluation(
+        GetTimestamp(
+          Literal(1580184371847000L),


Ok, I will replace three 0s with 123 in order to avoid mistake

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

TJX2014 · 2020-05-18T09:52:06Z

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

@@ -846,6 +853,8 @@ abstract class ToTimestamp
              case NonFatal(_) => null
            }
          }
+        case LongType =>
+          Math.multiplyExact(t.asInstanceOf[Long], scaleFactor.asInstanceOf[Long])


I will add a check when decide the value of scaleFactor to avoid the result of multiply out of range of Long value.

TJX2014 · 2020-05-18T10:11:42Z

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

+      checkExceptionInExpression[IllegalArgumentException](
+        GetTimestamp(
+          Literal(1580184371847000L),
+          Literal("other")),


@bart-samwel Could you please give me an example about what a non-literal means.

TJX2014 · 2020-05-18T10:28:43Z

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

+          Literal("milli")), 1580184371847000000L)
+      checkEvaluation(
+        GetTimestamp(
+          Literal(1580184371847000L),


You are right, I will add negative test.

TJX2014 · 2020-05-18T12:12:00Z

@bart-samwel a patch is added, Could you please help me review it.

MaxGekk · 2020-05-18T12:38:56Z

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

+        case LongType =>
+          val input = t.asInstanceOf[Long]
+          if ( minRange < input && input < maxRange ) {
+            Math.multiplyExact(input, scaleFactor.asInstanceOf[Long])


Math.multiplyExact throws ArithmeticException. You don't need to explicitly check the ranges.

My earlier concern was about whether we support the full range of Long as valid timestamps, or if the actual supported range is smaller. I googled a bit, and it seems that the full range is supported. So that's fine then without checks.

MaxGekk · 2020-05-18T12:39:55Z

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

@@ -806,6 +806,17 @@ abstract class ToTimestamp
      case NonFatal(_) => null
    }

+  private lazy val scaleFactor = right.toString match {


Please, look at other places in the file how to handle foldable expressions.

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

MaxGekk · 2020-05-18T12:41:59Z

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

@@ -789,7 +789,7 @@ abstract class ToTimestamp
  protected def downScaleFactor: Long

  override def inputTypes: Seq[AbstractDataType] =
-    Seq(TypeCollection(StringType, DateType, TimestampType), StringType)
+    Seq(TypeCollection(StringType, DateType, TimestampType, LongType ), StringType)


Why only LongType. Can be any IntegralType

Yes, IntegerType should also be considered.

MaxGekk · 2020-05-18T12:43:04Z

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

+          $javaType ${ev.value} = ${CodeGenerator.defaultValue(dataType)};
+          if (!${ev.isNull}) {
+            if ( (${minRange}L < ${eval1.value}) && (${eval1.value} < ${maxRange}L) ) {
+              ${ev.value} = ${eval1.value} * $scaleFactor;


Inconsistent with non-codegen where you use Math.multiplyExact

MaxGekk · 2020-05-18T12:43:44Z

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

@@ -846,6 +857,15 @@ abstract class ToTimestamp
              case NonFatal(_) => null
            }
          }
+        case LongType =>
+          val input = t.asInstanceOf[Long]
+          if ( minRange < input && input < maxRange ) {


Just in case, why do you exclude min and max values?

Yes, min and max should also be considered as correct input.Thanks for your advise.

MaxGekk · 2020-05-18T12:47:18Z

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

+              "input [" + ${eval1.value} + "] not from " + ${minRange}L + " to "
+               + ${maxRange}L + " for format " + "${right.toString}");


hmm, does this string interpolation work in Java?

cloud-fan · 2020-05-18T13:23:10Z

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

+  test("SPARK-31710:Fix millisecond and microsecond convert to timestamp in to_timestamp") {
+    withSQLConf() {
+      checkEvaluation(
+        GetTimestamp(


I thought we are going to add 3 new functions: TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS, to follow big query. Why do we overload GetTimestamp? Passing unit string as parameter is fragile, as we need to define/document the supported units and the behavior of invalid units.

I think your idea is right, I will add the three functions later and make a revert.

bart-samwel · 2020-05-18T15:01:35Z

I'd like to cross reference this with PR #28568 which is trying to solve this same issue with a compatibility flag. I think those two solutions are orthogonal -- there can be a flag to change the behavior (to easy migration from Hive), and there can be a better way to specify the conversion type explicitly. So I think the direction of this PR should go in no matter what happens with the other PR.

SparkQA · 2020-05-18T18:16:02Z

Test build #122804 has finished for PR 28534 at commit c2be46b.

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

cloud-fan · 2020-05-19T13:03:40Z

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

@@ -401,6 +401,92 @@ case class DayOfYear(child: Expression) extends UnaryExpression with ImplicitCas
  }
 }

+@ExpressionDescription(
+  usage = "_FUNC_(date) - Returns timestamp from seconds.",


_FUNC_(seconds) - Creates timestamp from the number of seconds since UTC epoch.

cloud-fan · 2020-05-19T13:04:22Z

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

+}
+
+@ExpressionDescription(
+  usage = "_FUNC_(date) - Returns timestamp from milliseconds.",


_FUNC_(seconds) - Creates timestamp from the number of milliseconds since UTC epoch.

cloud-fan · 2020-05-19T13:04:32Z

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

+}
+
+@ExpressionDescription(
+  usage = "_FUNC_(date) - Returns timestamp from microseconds.",


cloud-fan · 2020-05-19T13:06:26Z

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

+
+  protected def upScaleFactor: Long
+
+  override def inputTypes: Seq[AbstractDataType] = Seq(LongType, IntegerType)


we should accept LongType only. The analyzer will cast the input to long for you, if possible.

…amp_milliseconds to timestamp_millis

SparkQA · 2020-05-22T12:15:53Z

Test build #122972 has finished for PR 28534 at commit 7a54ffb.

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

SparkQA · 2020-05-22T12:20:29Z

Test build #122974 has finished for PR 28534 at commit fba8827.

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

SparkQA · 2020-05-22T13:48:50Z

Test build #122987 has finished for PR 28534 at commit f0cc631.

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

cloud-fan · 2020-05-22T14:16:25Z

thanks, merging to master!

cloud-fan · 2020-05-22T14:26:19Z

Hi @TJX2014 , the original JIRA doesn't fit this PR very well. Can you create a new JIRA ticket and update the PR title? Thanks!

TJX2014 · 2020-05-22T14:31:30Z

Thanks very much. Got a lot.

TJX2014 · 2020-05-22T14:32:29Z

Hi @TJX2014 , the original JIRA doesn't fit this PR very well. Can you create a new JIRA ticket and update the PR title? Thanks!

@cloud-fan ok, I will create the jira.

### What changes were proposed in this pull request? This is a followup of #28534 , to make `TIMESTAMP_SECONDS` function support fractional input as well. ### Why are the changes needed? Previously the cast function can cast fractional values to timestamp. Now we suggest users to ues these new functions, and we need to cover all the cast use cases. ### Does this PR introduce _any_ user-facing change? Yes, now `TIMESTAMP_SECONDS` function accepts fractional input. ### How was this patch tested? new tests Closes #28956 from cloud-fan/follow. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…X_MICROS ### What changes were proposed in this pull request? As #28534 adds functions from [BigQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/timestamp_functions) for converting numbers to timestamp, this PR is to add functions UNIX_SECONDS, UNIX_MILLIS and UNIX_MICROS for converting timestamp to numbers. ### Why are the changes needed? 1. Symmetry of the conversion functions 2. Casting timestamp type to numeric types is disallowed in ANSI mode, we should provide functions for users to complete the conversion. ### Does this PR introduce _any_ user-facing change? 3 new functions UNIX_SECONDS, UNIX_MILLIS and UNIX_MICROS for converting timestamp to long type. ### How was this patch tested? Unit tests. Closes #30566 from gengliangwang/timestampLong. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

probot-autolabeler bot added the SQL label May 15, 2020

TJX2014 force-pushed the master-SPARK-31710 branch 2 times, most recently from 487197b to 39eac6c Compare May 16, 2020 10:20

TJX2014 changed the title ~~[SPARK-31710][SQL]Fix timestamp and long convert not compatible with hive issue~~ [SPARK-31710][SQL]Fix millisecond and microsecond convert to timestamp in to_timestamp May 16, 2020

bart-samwel reviewed May 18, 2020

View reviewed changes

TJX2014 force-pushed the master-SPARK-31710 branch from 019d6b2 to c2be46b Compare May 18, 2020 12:05

TJX2014 commented May 18, 2020

View reviewed changes

MaxGekk reviewed May 18, 2020

View reviewed changes

cloud-fan reviewed May 18, 2020

View reviewed changes

bart-samwel mentioned this pull request May 18, 2020

[SPARK-31710][SQL]Add compatibility flag to cast long to timestamp #28568

Closed

TJX2014 force-pushed the master-SPARK-31710 branch from c2be46b to 0355ce4 Compare May 19, 2020 12:30

TJX2014 changed the title ~~[SPARK-31710][SQL]Fix millisecond and microsecond convert to timestamp in to_timestamp~~ [SPARK-31710][SQL]TIMESTAMP_SECONDS, TIMESTAMP_MILLISECONDS and TIMESTAMP_MICROSECONDS to timestamp transfer May 19, 2020

TJX2014 requested a review from cloud-fan May 19, 2020 12:47

cloud-fan reviewed May 19, 2020

View reviewed changes

TJX2014 added 13 commits May 22, 2020 16:24

long overflow in datetime.sql added and pass the local test

17a33f5

test overflow condition fix, just small unit for per test

344e5e1

fix unit test failed due to string value in ExpressionDescription

407dcfc

regenerate sql-expression-schema.md

944c644

minimize code gen by judge equal 1 and remove duplicate unit test

642668c

correct style of usage content

62bf8b5

short name to MillisToTimestamp and MicrosToTimestamp

7da94b5

correct short name in sql-expression-schema.md

3704fee

function rename timestamp_microseconds to timestamp_micros and timest…

f5b0a5a

…amp_milliseconds to timestamp_millis

disable cast string to long in number to timestamp

f4f7c35

regenerate sql-expression-schema.md, UT added in DateExpressionsSuite

3c949ac

UT style improve

fdfd75c

UT style improve

f0cc631

TJX2014 force-pushed the master-SPARK-31710 branch from fba8827 to f0cc631 Compare May 22, 2020 08:39

cloud-fan approved these changes May 22, 2020

View reviewed changes

HyukjinKwon approved these changes May 22, 2020

View reviewed changes

cloud-fan closed this in 2115c55 May 22, 2020

TJX2014 changed the title ~~[SPARK-31710][SQL] Adds TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS functions~~ [SPARK-31797][SQL] Adds TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS functions May 22, 2020

cloud-fan mentioned this pull request Jun 30, 2020

[SPARK-31797][SQL][FOLLOWUP] TIMESTAMP_SECONDS supports fractional input #28956

Closed

gengliangwang mentioned this pull request Dec 2, 2020

[SPARK-33627][SQL] Add new function UNIX_SECONDS, UNIX_MILLIS and UNIX_MICROS #30566

Closed

thirtiseven mentioned this pull request Jul 3, 2023

Support TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS [databricks] NVIDIA/spark-rapids#8650

Merged

		"input [" + ${eval1.value} + "] not from " + ${minRange}L + " to "
		+ ${maxRange}L + " for format " + "${right.toString}");


		protected def upScaleFactor: Long

		override def inputTypes: Seq[AbstractDataType] = Seq(LongType, IntegerType)

[SPARK-31797][SQL] Adds TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS functions #28534

[SPARK-31797][SQL] Adds TIMESTAMP_SECONDS, TIMESTAMP_MILLIS and TIMESTAMP_MICROS functions #28534

Conversation

TJX2014 commented May 15, 2020 • edited Loading

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?

bart-samwel commented May 15, 2020

TJX2014 commented May 15, 2020

cloud-fan commented May 15, 2020

TJX2014 commented May 15, 2020

TJX2014 commented May 16, 2020 • edited Loading

cloud-fan commented May 17, 2020

SparkQA commented May 17, 2020

SparkQA commented May 18, 2020

TJX2014 commented May 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 18, 2020

TJX2014 left a 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

Choose a reason for hiding this comment

TJX2014 commented May 18, 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

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 May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart-samwel commented May 18, 2020

SparkQA commented May 18, 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 May 22, 2020

SparkQA commented May 22, 2020

SparkQA commented May 22, 2020

cloud-fan commented May 22, 2020

cloud-fan commented May 22, 2020

TJX2014 commented May 22, 2020

TJX2014 commented May 22, 2020

TJX2014 commented May 15, 2020 •

edited

Loading

TJX2014 commented May 16, 2020 •

edited

Loading

cloud-fan May 18, 2020 •

edited

Loading