You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/05/18 12:33:45 UTC
[GitHub] [spark] MaxGekk commented on a change in pull request #28570: [SPARK-31710][SQL]Add compatibility flag to cast long to timestamp
MaxGekk commented on a change in pull request #28570:
URL: https://github.com/apache/spark/pull/28570#discussion_r426587553
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -453,8 +453,11 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
if (d.isNaN || d.isInfinite) null else (d * MICROS_PER_SECOND).toLong
}
- // converting seconds to us
- private[this] def longToTimestamp(t: Long): Long = SECONDS.toMicros(t)
+ // SPARK-31710 converting seconds to us,Add compatibility flag
+ private[this] def longToTimestamp(t: Long): Long = {
+ if ( SQLConf.get.getConf( SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS )) t * 1000000L
Review comment:
I guess checking the flag per each value brings significant performance drop. Could you show by a benchmark that performance is not sacrificed by your changes.
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
##########
@@ -1353,17 +1353,33 @@ class AnsiCastSuite extends CastSuiteBase {
cast("abc.com", dataType), "invalid input")
}
}
-
- test("cast a timestamp before the epoch 1970-01-01 00:00:00Z") {
Review comment:
Why do you delete the test?
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala
##########
@@ -453,8 +453,11 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
if (d.isNaN || d.isInfinite) null else (d * MICROS_PER_SECOND).toLong
}
- // converting seconds to us
- private[this] def longToTimestamp(t: Long): Long = SECONDS.toMicros(t)
+ // SPARK-31710 converting seconds to us,Add compatibility flag
+ private[this] def longToTimestamp(t: Long): Long = {
+ if ( SQLConf.get.getConf( SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS )) t * 1000000L
+ else t * 1000L
Review comment:
There are constants in `DateTimeConstants`. Please, use them.
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
##########
@@ -1353,17 +1353,33 @@ class AnsiCastSuite extends CastSuiteBase {
cast("abc.com", dataType), "invalid input")
}
}
-
- test("cast a timestamp before the epoch 1970-01-01 00:00:00Z") {
- def errMsg(t: String): String = s"Casting -2198208303900000 to $t causes overflow"
- withDefaultTimeZone(UTC) {
- val negativeTs = Timestamp.valueOf("1900-05-05 18:34:56.1")
- assert(negativeTs.getTime < 0)
- val expectedSecs = Math.floorDiv(negativeTs.getTime, MILLIS_PER_SECOND)
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, ByteType), errMsg("byte"))
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, ShortType), errMsg("short"))
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, IntegerType), errMsg("int"))
- checkEvaluation(cast(negativeTs, LongType), expectedSecs)
+
+ test("SPARK-31710:Add compatibility flag to cast long to timestamp") {
+ withSQLConf(
+ SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS.key -> "false") {
+ for (tz <- ALL_TIMEZONES) {
+ def checkLongToTimestamp(str: Long, expected: Long): Unit = {
+ checkEvaluation(cast(str, TimestampType, Option(tz.getID)), expected)
+ }
+ checkLongToTimestamp(253402272000L, 253402272000000L)
+ checkLongToTimestamp(-5L, -5000L)
+ checkLongToTimestamp(1L, 1000L)
+ checkLongToTimestamp(0L, 0L)
+ checkLongToTimestamp(123L, 123000L)
+ }
+ }
+ withSQLConf(
+ SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS.key -> "true") {
+ for (tz <- ALL_TIMEZONES) {
+ def checkLongToTimestamp(str: Long, expected: Long): Unit = {
+ checkEvaluation(cast(str, TimestampType, Option(tz.getID)), expected)
+ }
+ checkLongToTimestamp(253402272000L, 253402272000000000L)
+ checkLongToTimestamp(-5L, -5000000L)
+ checkLongToTimestamp(1L, 1000000L)
+ checkLongToTimestamp(0L, 0L)
+ checkLongToTimestamp(123L, 123000000L)
+ }
}
}
-}
+}
Review comment:
new line
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
##########
@@ -1353,17 +1353,33 @@ class AnsiCastSuite extends CastSuiteBase {
cast("abc.com", dataType), "invalid input")
}
}
-
- test("cast a timestamp before the epoch 1970-01-01 00:00:00Z") {
- def errMsg(t: String): String = s"Casting -2198208303900000 to $t causes overflow"
- withDefaultTimeZone(UTC) {
- val negativeTs = Timestamp.valueOf("1900-05-05 18:34:56.1")
- assert(negativeTs.getTime < 0)
- val expectedSecs = Math.floorDiv(negativeTs.getTime, MILLIS_PER_SECOND)
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, ByteType), errMsg("byte"))
Review comment:
No errors while casting to byte anymore in ANSI mode?
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
##########
@@ -1353,17 +1353,33 @@ class AnsiCastSuite extends CastSuiteBase {
cast("abc.com", dataType), "invalid input")
}
}
-
- test("cast a timestamp before the epoch 1970-01-01 00:00:00Z") {
- def errMsg(t: String): String = s"Casting -2198208303900000 to $t causes overflow"
- withDefaultTimeZone(UTC) {
- val negativeTs = Timestamp.valueOf("1900-05-05 18:34:56.1")
- assert(negativeTs.getTime < 0)
- val expectedSecs = Math.floorDiv(negativeTs.getTime, MILLIS_PER_SECOND)
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, ByteType), errMsg("byte"))
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, ShortType), errMsg("short"))
- checkExceptionInExpression[ArithmeticException](cast(negativeTs, IntegerType), errMsg("int"))
- checkEvaluation(cast(negativeTs, LongType), expectedSecs)
+
+ test("SPARK-31710:Add compatibility flag to cast long to timestamp") {
+ withSQLConf(
+ SQLConf.LONG_TIMESTAMP_CONVERSION_IN_SECONDS.key -> "false") {
+ for (tz <- ALL_TIMEZONES) {
Review comment:
What is the reason to test all time zones?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org