Skip to content

[SPARK-10439] [sql] Add bound checks to DateTimeUtils. #8606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vanzin wants to merge 10 commits into from
Closed

[SPARK-10439] [sql] Add bound checks to DateTimeUtils. #8606

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ffe39e4
[SPARK-10439] [sql] Add bound checks to DateTimeUtils.
Sep 4, 2015
196ba9e
Merge branch 'master' into SPARK-10439
Sep 8, 2015
63d0c39
Revert to previous bounds for timestamp values.
Sep 8, 2015
00757d2
Merge branch 'master' into SPARK-10439
Sep 9, 2015
edeb06e
Revert to previous limits, and explain why they're needed.
Sep 9, 2015
922c09e
Remove unneded checks.
Sep 9, 2015
3869816
Fix negative timestamp to julian day ts conversion.
Sep 9, 2015
6064ef2
Fix timestamp range check, add check to fromJavaDate().
Sep 9, 2015
7a4cf9e
And a test.
Sep 9, 2015
fa8d6f6
Simplify toJulianDay().
Sep 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.spark.sql.catalyst.util

import java.lang.{Long => JLong}
import java.sql.{Date, Timestamp}
import java.text.{DateFormat, SimpleDateFormat}
import java.util.{TimeZone, Calendar}
Expand Down Expand Up @@ -56,6 +57,12 @@ object DateTimeUtils {

@transient lazy val defaultTimeZone = TimeZone.getDefault

// Constants defining the allowed ranges for timestampts that fit in parquet files. Mostly
// used by tests to avoid duplication.
private[spark] final val MIN_TIMESTAMP: Long =
-(DateTimeUtils.JULIAN_DAY_OF_EPOCH * DateTimeUtils.SECONDS_PER_DAY * 1000)
Copy link
Contributor

@yhuai yhuai Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave it a try and I got

scala> new java.sql.Timestamp(-4708372992000L)
res3: java.sql.Timestamp = 1820-10-18 14:36:48.0

scala> java.sql.Timestamp.valueOf("0317-02-14 06:13:20.0")
res7: java.sql.Timestamp = 0317-02-14 06:13:20.0

scala> res5.getTime
res8: Long = -52159715200000

scala> new java.sql.Timestamp(-72135740800000L)
res1: java.sql.Timestamp = 0317-02-14 06:13:20.0

So, is it the right lower bound?

Copy link
Contributor Author

@vanzin vanzin Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the right value if you consider the math. But I've seen really weird behavior in how the Java classes print very large (positive or negative) timestamps, and I didn't find anything in the javadocs about limits, so I'm not sure what's the best way to proceed. We can choose an arbitrary minimum and maximum, but what would those be based on?

For example, java.sql.Timestamp will print the same formatted date string for both -793495812000 and -123456789012000L.

private[spark] final val MAX_TIMESTAMP: Long = java.lang.Long.MAX_VALUE / 1000
Copy link
Contributor

@yhuai yhuai Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, although we can do

scala> new java.sql.Timestamp(java.lang.Long.MAX_VALUE / 1000)
res13: java.sql.Timestamp = 294247-01-09 20:00:54.775

but, we cannot do java.sql.Timestamp.valueOf("294247-01-09 20:00:54.775"). There is

java.lang.IllegalArgumentException: Timestamp format must be yyyy-mm-dd hh:mm:ss[.fffffffff]
    at java.sql.Timestamp.valueOf(Timestamp.java:235)
    at .<init>(<console>:8)
    at .<clinit>(<console>)
    at .<init>(<console>:7)
    at .<clinit>(<console>)
    at $print(<console>)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at scala.tools.nsc.interpreter.IMain$ReadEvalPrint.call(IMain.scala:734)
    at scala.tools.nsc.interpreter.IMain$Request.loadAndRun(IMain.scala:983)
    at scala.tools.nsc.interpreter.IMain.loadAndRunReq$1(IMain.scala:573)
    at scala.tools.nsc.interpreter.IMain.interpret(IMain.scala:604)
    at scala.tools.nsc.interpreter.IMain.interpret(IMain.scala:568)
    at scala.tools.nsc.interpreter.ILoop.reallyInterpret$1(ILoop.scala:760)
    at scala.tools.nsc.interpreter.ILoop.interpretStartingWith(ILoop.scala:805)
    at scala.tools.nsc.interpreter.ILoop.command(ILoop.scala:717)
    at scala.tools.nsc.interpreter.ILoop.processLine$1(ILoop.scala:581)
    at scala.tools.nsc.interpreter.ILoop.innerLoop$1(ILoop.scala:588)
    at scala.tools.nsc.interpreter.ILoop.loop(ILoop.scala:591)
    at scala.tools.nsc.interpreter.ILoop$$anonfun$process$1.apply$mcZ$sp(ILoop.scala:882)
    at scala.tools.nsc.interpreter.ILoop$$anonfun$process$1.apply(ILoop.scala:837)
    at scala.tools.nsc.interpreter.ILoop$$anonfun$process$1.apply(ILoop.scala:837)
    at scala.tools.nsc.util.ScalaClassLoader$.savingContextLoader(ScalaClassLoader.scala:135)
    at scala.tools.nsc.interpreter.ILoop.process(ILoop.scala:837)
    at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:83)
    at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:96)
    at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:105)
    at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)

For text based format, the upper bound of timestamp value is much smaller.

Copy link
Contributor Author

@vanzin vanzin Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the ranges might be different for the string parser, but is there any documentation about what that range would be?


// Java TimeZone has no mention of thread safety. Use thread local instance to be safe.
private val threadLocalLocalTimeZone = new ThreadLocal[TimeZone] {
override protected def initialValue: TimeZone = {
Expand All @@ -82,7 +89,9 @@ object DateTimeUtils {
// SPARK-6785: use Math.floor so negative number of days (dates before 1970)
// will correctly work as input for function toJavaDate(Int)
val millisLocal = millisUtc + threadLocalLocalTimeZone.get().getOffset(millisUtc)
Math.floor(millisLocal.toDouble / MILLIS_PER_DAY).toInt
val days = Math.floor(millisLocal.toDouble / MILLIS_PER_DAY)
require(days <= Integer.MAX_VALUE && days >= Integer.MIN_VALUE, "Date exceeeds allowed range.")
Copy link
Contributor

@davies davies Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume the millisUtc is a valid timestamp, then this check is not needed.

days.toInt
}

// reverse of millisToDays
Expand Down Expand Up @@ -172,6 +181,8 @@ object DateTimeUtils {
*/
def fromJavaTimestamp(t: Timestamp): SQLTimestamp = {
if (t != null) {
require(t.getTime() <= JLong.MAX_VALUE / 1000 && t.getTime() >= JLong.MIN_VALUE / 1000,
s"Timestamp exceeds allowed range (${t.getTime()}).")
t.getTime() * 1000L + (t.getNanos().toLong / 1000) % 1000L
} else {
0L
Expand All @@ -193,9 +204,15 @@ object DateTimeUtils {
*/
def toJulianDay(us: SQLTimestamp): (Int, Long) = {
val seconds = us / MICROS_PER_SECOND
val day = seconds / SECONDS_PER_DAY + JULIAN_DAY_OF_EPOCH
val secondsInDay = seconds % SECONDS_PER_DAY
val nanos = (us % MICROS_PER_SECOND) * 1000L
var day = seconds / SECONDS_PER_DAY + JULIAN_DAY_OF_EPOCH
var secondsInDay = seconds % SECONDS_PER_DAY
var nanos = (us % MICROS_PER_SECOND) * 1000L
if (us < 0) {
Copy link
Contributor

@davies davies Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

day -= 1
secondsInDay += SECONDS_PER_DAY
nanos += (SECONDS_PER_DAY * MICROS_PER_SECOND * 1000L)
Copy link
Contributor

@davies davies Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, it should be:


val julian_us = us + JULIAN_DAY_OF_EPOCH * MICROSECONDS_PER_DAY
val day = julian_us / MICROSECONDS_PER_DAY
val micros = julian_us % MICROSECONDS_PER_DAY
(day.toInt, micros * 1000L)

Copy link
Contributor Author

@vanzin vanzin Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; my code can return nanos > 999999999 which java.sql.Timestamp does not like...

Copy link
Contributor

@davies davies Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created https://issues.apache.org/jira/browse/SPARK-10522 for this, then we could back port the fix for 1.5.

require(day >= 0, "Timestamp exceeds allowed range.")
}
(day.toInt, secondsInDay * NANOS_PER_SECOND + nanos)
}

Expand Down
22 changes: 6 additions & 16 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/RandomDataGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import scala.util.Random
import org.apache.spark.sql.catalyst.CatalystTypeConverters
import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.sql.types._
import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.unsafe.types.CalendarInterval

/**
Expand Down Expand Up @@ -109,22 +110,11 @@ object RandomDataGenerator {
})
case BooleanType => Some(() => rand.nextBoolean())
case DateType => Some(() => new java.sql.Date(rand.nextInt()))
case TimestampType =>
val generator =
() => {
var milliseconds = rand.nextLong() % 253402329599999L
// -62135740800000L is the number of milliseconds before January 1, 1970, 00:00:00 GMT
// for "0001-01-01 00:00:00.000000". We need to find a
// number that is greater or equals to this number as a valid timestamp value.
while (milliseconds < -62135740800000L) {
// 253402329599999L is the the number of milliseconds since
// January 1, 1970, 00:00:00 GMT for "9999-12-31 23:59:59.999999".
milliseconds = rand.nextLong() % 253402329599999L
}
// DateTimeUtils.toJavaTimestamp takes microsecond.
DateTimeUtils.toJavaTimestamp(milliseconds * 1000)
}
Some(generator)
case TimestampType => Some { () =>
val range = DateTimeUtils.MAX_TIMESTAMP - DateTimeUtils.MIN_TIMESTAMP
val milliseconds = (range * rand.nextDouble()).toLong + DateTimeUtils.MIN_TIMESTAMP
DateTimeUtils.toJavaTimestamp(milliseconds * 1000)
}
case CalendarIntervalType => Some(() => {
val months = rand.nextInt(1000)
val ns = rand.nextLong()
Expand Down
5 changes: 4 additions & 1 deletion sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/LiteralGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import org.scalatest.Matchers
import org.scalatest.prop.GeneratorDrivenPropertyChecks

import org.apache.spark.sql.types._
import org.apache.spark.sql.catalyst.util.DateTimeUtils
import org.apache.spark.unsafe.types.CalendarInterval

/**
Expand Down Expand Up @@ -94,7 +95,9 @@ object LiteralGenerator {
for { d <- Arbitrary.arbInt.arbitrary } yield Literal.create(new Date(d), DateType)

lazy val timestampLiteralGen: Gen[Literal] =
for { t <- Arbitrary.arbLong.arbitrary } yield Literal.create(new Timestamp(t), TimestampType)
for { t <- Gen.chooseNum(DateTimeUtils.MIN_TIMESTAMP, DateTimeUtils.MAX_TIMESTAMP) } yield {
Literal.create(new Timestamp(t), TimestampType)
}

lazy val calendarIntervalLiterGen: Gen[Literal] =
for { m <- Arbitrary.arbInt.arbitrary; s <- Arbitrary.arbLong.arbitrary}
Expand Down
31 changes: 31 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.spark.sql.catalyst.util

import java.lang.{Long => JLong}
import java.sql.{Date, Timestamp}
import java.text.SimpleDateFormat
import java.util.{Calendar, TimeZone}
Expand Down Expand Up @@ -425,4 +426,34 @@ class DateTimeUtilsSuite extends SparkFunSuite {
test("2011-12-25 01:00:00.123456", "PST", "2011-12-25 09:00:00.123456")
test("2011-12-25 17:00:00.123456", "Asia/Shanghai", "2011-12-25 09:00:00.123456")
}

test("SPARK-10439: bound checks") {
// Avoid truncation when converting from ms to us.
Seq(JLong.MIN_VALUE, JLong.MAX_VALUE).foreach { ts =>
intercept[IllegalArgumentException] {
fromJavaTimestamp(new Timestamp(ts))
}
}

// Avoid negative Julian day counts since the Hive/Impala timestamp spec does not
// expect them.
val julianDay = -(JULIAN_DAY_OF_EPOCH * SECONDS_PER_DAY * MICROS_PER_SECOND)
intercept[IllegalArgumentException] {
toJulianDay(julianDay - 1)
}

// Make sure calculated nanos are positive, since that's what the Hive/Impala timestamp
// spec expects.
val (_, nanos) = toJulianDay(julianDay + 1)
assert(nanos >= 0)

// Make sure timestamp in ms does not overflow number of days returned as int. millisToDays()
// considers the current time zone, so add 2 full days when checking to be sure.
val msPerDay = SECONDS_PER_DAY * 1000L
val tooManyDays = Integer.MAX_VALUE * msPerDay
intercept[IllegalArgumentException] {
millisToDays(tooManyDays + 2 * msPerDay)
}
}

}