bart-samwel commented on a change in pull request #28534:
URL: https://github.com/apache/spark/pull/28534#discussion_r426483766
##########
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:
```suggestion
"format must be 'milli' or 'micro' when converting from Long type;
specified format is '" + o + "'")
```
##########
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:
Test with literals that don't end in three 0s?
##########
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:
Also test with negative (pre-Unix epoch) timestamps.
##########
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:
@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.
##########
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:
Also test with a non-literal input. The error should still make sense
then.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
