TJX2014 commented on a change in pull request #28534:
URL: https://github.com/apache/spark/pull/28534#discussion_r426526568
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
##########
@@ -1146,4 +1146,23 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
Literal("yyyy-MM-dd'T'HH:mm:ss.SSSz")), "Fail to parse")
}
}
+
+ test("SPARK-31710:Fix millisecond and microsecond convert to timestamp in
to_timestamp") {
+ withSQLConf() {
+ checkEvaluation(
+ GetTimestamp(
+ Literal(1580184371847000L),
+ Literal("milli")), 1580184371847000000L)
+ checkEvaluation(
+ GetTimestamp(
+ Literal(1580184371847000L),
Review comment:
You are right, I will add negative test.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala
##########
@@ -806,6 +806,13 @@ abstract class ToTimestamp
case NonFatal(_) => null
}
+ private lazy val scaleFactor = right.toString match {
+ case "milli" => MICROS_PER_MILLIS
+ case "micro" => 1L
+ case o => throw new IllegalArgumentException(
+ "current param is '" + o + "'" + ";param must be 'milli' or 'micro'
when use Long type time")
Review comment:
Nice job
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
##########
@@ -1146,4 +1146,23 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
Literal("yyyy-MM-dd'T'HH:mm:ss.SSSz")), "Fail to parse")
}
}
+
+ test("SPARK-31710:Fix millisecond and microsecond convert to timestamp in
to_timestamp") {
+ withSQLConf() {
+ checkEvaluation(
+ GetTimestamp(
+ Literal(1580184371847000L),
+ Literal("milli")), 1580184371847000000L)
+ checkEvaluation(
+ GetTimestamp(
+ Literal(1580184371847000L),
+ Literal("micro")), 1580184371847000L)
+ checkExceptionInExpression[IllegalArgumentException](
+ GetTimestamp(
+ Literal(1580184371847000L),
+ Literal("other")),
Review comment:
@bart-samwel Could you please give me an example about what a
non-literal means.
##########
File path:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
##########
@@ -1146,4 +1146,23 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
Literal("yyyy-MM-dd'T'HH:mm:ss.SSSz")), "Fail to parse")
}
}
+
+ test("SPARK-31710:Fix millisecond and microsecond convert to timestamp in
to_timestamp") {
+ withSQLConf() {
+ checkEvaluation(
+ GetTimestamp(
+ Literal(1580184371847000L),
Review comment:
Ok, I will replace three 0s with 123 in order to avoid mistake
##########
File path:
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])
Review comment:
I will add a check when decide the value of `scaleFactor` to avoid the
result of multiply out of range of Long value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
