Skip to content

[SPARK-15613] [SQL] Fix incorrect days to millis conversion due to Daylight Saving Time #13652

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
davies wants to merge 5 commits into from
Closed

[SPARK-15613] [SQL] Fix incorrect days to millis conversion due to Daylight Saving Time #13652

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20904c4
fix toUTCTime and daysToMillis
Jun 13, 2016
ac3d2b5
cover all timezone
Jun 14, 2016
f23b545
more acurate
Jun 15, 2016
9df8c84
skip some days in tests
Jun 16, 2016
88df176
fallback to java.util.Date
Jun 18, 2016
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
20 changes: 17 additions & 3 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 @@ -100,8 +100,8 @@ object DateTimeUtils {

// reverse of millisToDays
def daysToMillis(days: SQLDate): Long = {
val millisUtc = days.toLong * MILLIS_PER_DAY
millisUtc - threadLocalLocalTimeZone.get().getOffset(millisUtc)
val millisLocal = days.toLong * MILLIS_PER_DAY
millisLocal - getOffsetFromLocalMillis(millisLocal, threadLocalLocalTimeZone.get())
}

def dateToString(days: SQLDate): String =
Expand Down Expand Up @@ -850,6 +850,20 @@ object DateTimeUtils {
}
}

/**
* Lookup the offset for given millis seconds since 1970-01-01 00:00:00 in a timezone.
Copy link
Contributor

@adrian-wang adrian-wang Jun 16, 2016

Choose a reason for hiding this comment

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

since 1970-01-01 00:00:00 UTC or local timezone?

*/
private def getOffsetFromLocalMillis(millisLocal: Long, tz: TimeZone): Long = {
val rawOffset = tz.getRawOffset
// the actual offset should be calculated based on milliseconds in UTC
var offset = tz.getOffset(millisLocal - rawOffset)
if (offset != rawOffset) {
// Could be in DST, try with best effort
offset = tz.getOffset(millisLocal - offset)
}
offset
}

/**
* Returns a timestamp of given timezone from utc timestamp, with the same string
* representation in their timezone.
Expand All @@ -866,7 +880,7 @@ object DateTimeUtils {
*/
def toUTCTime(time: SQLTimestamp, timeZone: String): SQLTimestamp = {
val tz = TimeZone.getTimeZone(timeZone)
val offset = tz.getOffset(time / 1000L)
val offset = getOffsetFromLocalMillis(time / 1000L, tz)
time - offset * 1000L
}
}
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/types/DateType.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import org.apache.spark.sql.catalyst.ScalaReflectionLock
*
* Please use the singleton [[DataTypes.DateType]].
*
* Internally, this is represented as the number of days from epoch (1970-01-01 00:00:00 UTC).
* Internally, this is represented as the number of days from 1970-01-01.
Copy link
Contributor

@JoshRosen JoshRosen Jun 13, 2016

Choose a reason for hiding this comment

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

Can this comment more explicitly say that this is time-zone sensitive / with respect to the local time zone for some definition of "local"?

Copy link
Contributor Author

@davies davies Jun 14, 2016

Choose a reason for hiding this comment

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

The DateType here has nothing with timezone, timezone is considered only when DateType is converted to/from TimestampType, right?

*/
@DeveloperApi
class DateType private() extends AtomicType {
Expand Down
27 changes: 27 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 @@ -492,6 +492,13 @@ class DateTimeUtilsSuite extends SparkFunSuite {
test("2011-12-25 09:00:00.123456", "JST", "2011-12-25 18:00:00.123456")
test("2011-12-25 09:00:00.123456", "PST", "2011-12-25 01:00:00.123456")
test("2011-12-25 09:00:00.123456", "Asia/Shanghai", "2011-12-25 17:00:00.123456")

// Daylight Saving Time
test("2016-03-13 09:59:59.0", "PST", "2016-03-13 01:59:59.0")
test("2016-03-13 10:00:00.0", "PST", "2016-03-13 03:00:00.0")
test("2016-11-06 08:59:59.0", "PST", "2016-11-06 01:59:59.0")
test("2016-11-06 09:00:00.0", "PST", "2016-11-06 01:00:00.0")
test("2016-11-06 10:00:00.0", "PST", "2016-11-06 02:00:00.0")
}

test("to UTC timestamp") {
Expand All @@ -503,5 +510,25 @@ class DateTimeUtilsSuite extends SparkFunSuite {
test("2011-12-25 18:00:00.123456", "JST", "2011-12-25 09:00:00.123456")
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")

// Daylight Saving Time
test("2016-03-13 01:59:59", "PST", "2016-03-13 09:59:59.0")
// 2016-03-13 02:00:00 PST does not exists
test("2016-03-13 02:00:00", "PST", "2016-03-13 10:00:00.0")
test("2016-03-13 03:00:00", "PST", "2016-03-13 10:00:00.0")
test("2016-11-06 00:59:59", "PST", "2016-11-06 07:59:59.0")
// 2016-11-06 01:00:00 PST could be 2016-11-06 08:00:00 UTC or 2016-11-06 09:00:00 UTC
test("2016-11-06 01:00:00", "PST", "2016-11-06 09:00:00.0")
test("2016-11-06 01:59:59", "PST", "2016-11-06 09:59:59.0")
test("2016-11-06 02:00:00", "PST", "2016-11-06 10:00:00.0")
}

test("daysToMillis and millisToDays") {
(-1001 to 2000).foreach { d =>
assert(millisToDays(daysToMillis(d)) === d)
Copy link
Contributor

@JoshRosen JoshRosen Jun 13, 2016

Choose a reason for hiding this comment

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

You may need to test this in multiple timezones, since this wouldn't have necessarily failed before in US Pacific. Switching the default timezone within a test case is a bit tricky, though: I have code for this in one of my branches.

Copy link
Contributor Author

@davies davies Jun 13, 2016

Choose a reason for hiding this comment

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

See that, will pull yours here.

val date = toJavaTimestamp(daysToMillis(d) * 1000L)
assert(date.getHours === 0)
assert(date.getMinutes === 0)
}
}
}