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 all commits
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
51 changes: 48 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -850,6 +850,41 @@ object DateTimeUtils {
}
}

/**
* Lookup the offset for given millis seconds since 1970-01-01 00:00:00 in given timezone.
*/
private def getOffsetFromLocalMillis(millisLocal: Long, tz: TimeZone): Long = {
var guess = tz.getRawOffset
// the actual offset should be calculated based on milliseconds in UTC
val offset = tz.getOffset(millisLocal - guess)
if (offset != guess) {
guess = tz.getOffset(millisLocal - offset)
if (guess != offset) {
// fallback to do the reverse lookup using java.sql.Timestamp
// this should only happen near the start or end of DST
val days = Math.floor(millisLocal.toDouble / MILLIS_PER_DAY).toInt
val year = getYear(days)
val month = getMonth(days)
val day = getDayOfMonth(days)

var millisOfDay = (millisLocal % MILLIS_PER_DAY).toInt
if (millisOfDay < 0) {
millisOfDay += MILLIS_PER_DAY.toInt
}
val seconds = (millisOfDay / 1000L).toInt
val hh = seconds / 3600
val mm = seconds / 60 % 60
val ss = seconds % 60
val nano = millisOfDay % 1000 * 1000000

// create a Timestamp to get the unix timestamp (in UTC)
val timestamp = new Timestamp(year - 1900, month - 1, day, hh, mm, ss, nano)
guess = (millisLocal - timestamp.getTime).toInt
}
}
guess
}

/**
* Returns a timestamp of given timezone from utc timestamp, with the same string
* representation in their timezone.
Expand All@@ -866,7 +901,17 @@ 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
}

/**
* Re-initialize the current thread's thread locals. Exposed for testing.
*/
private[util] def resetThreadLocals(): Unit = {
threadLocalGmtCalendar.remove()
threadLocalLocalTimeZone.remove()
threadLocalTimestampFormat.remove()
threadLocalDateFormat.remove()
}
}
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 numberDiff line numberDiff 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.

@JoshRosenJoshRosenJun 13, 2016

Copy link
Copy Markdown
Contributor

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"?

@daviesdaviesJun 14, 2016

Copy link
Copy Markdown
ContributorAuthor

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
40 changes: 40 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeTestUtils.scala
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

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

import java.util.TimeZone

/**
* Helper functions for testing date and time functionality.
*/
object DateTimeTestUtils {

val ALL_TIMEZONES: Seq[TimeZone] = TimeZone.getAvailableIDs.toSeq.map(TimeZone.getTimeZone)

def withDefaultTimeZone[T](newDefaultTimeZone: TimeZone)(block: => T): T = {
val originalDefaultTimeZone = TimeZone.getDefault
try {
DateTimeUtils.resetThreadLocals()
TimeZone.setDefault(newDefaultTimeZone)
block
} finally {
TimeZone.setDefault(originalDefaultTimeZone)
DateTimeUtils.resetThreadLocals()
}
}
}
40 changes: 40 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala
View file
Open in desktop
Original file line numberDiff line numberDiff 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,38 @@ 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") {
// There are some days are skipped entirely in some timezone, skip them here.
val skipped_days = Map[String, Int](
"Kwajalein" -> 8632,
"Pacific/Apia" -> 15338,
"Pacific/Enderbury" -> 9131,
"Pacific/Fakaofo" -> 15338,
"Pacific/Kiritimati" -> 9131,
"Pacific/Kwajalein" -> 8632,
"MIT" -> 15338)
for (tz <- DateTimeTestUtils.ALL_TIMEZONES) {
DateTimeTestUtils.withDefaultTimeZone(tz) {
val skipped = skipped_days.getOrElse(tz.getID, Int.MinValue)
(-20000 to 20000).foreach { d =>
if (d != skipped) {
assert(millisToDays(daysToMillis(d)) === d)
}
}

@ckadnerckadnerJun 16, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the range (-2000 to 2000) should be extended to cover more recent dates, today is day 16967 since the epoch, i.e.:

 for (tz <- DateTimeTestUtils.ALL_TIMEZONES) {
DateTimeTestUtils.withDefaultTimeZone(tz) {
(-20000 to 20000).foreach { d =>
assert(millisToDays(daysToMillis(d)) === d, s"toJavaDate($d) = ${toJavaDate(d)} in ${tz}")
}
}

...which will show more problem(s):

8633 did not equal 8632 toJavaDate(8632) = 1993-08-21 in sun.util.calendar.ZoneInfo[id="Kwajalein",offset=43200000,dstSavings=0,useDaylight=false,transitions=5,lastRule=null]

... list of the incorrectly converted days, with dates in time zones:

for (tz <- DateTimeTestUtils.ALL_TIMEZONES) {
DateTimeTestUtils.withDefaultTimeZone(tz) {
(-20000 to 20000).filterNot(d => d === millisToDays(daysToMillis(d))).foreach { d =>
println(s"toJavaDate($d) = ${toJavaDate(d)} in ${tz}")
}
}
}
toJavaDate(8632) = 1993-08-21 in time zone [id="Kwajalein",offset=43200000,dstSavings=0,useDaylight=false,transitions=5,lastRule=null]
toJavaDate(15338) = 2011-12-31 in time zone [id="Pacific/Apia",offset=46800000,dstSavings=3600000,useDaylight=true,transitions=59,lastRule=java.util.SimpleTimeZone[id=Pacific/Apia,offset=46800000,dstSavings=3600000,useDaylight=true,startYear=0,startMode=2,startMonth=8,startDay=-1,startDayOfWeek=1,startTime=10800000,startTimeMode=0,endMode=3,endMonth=3,endDay=1,endDayOfWeek=1,endTime=14400000,endTimeMode=0]]
toJavaDate(9131) = 1995-01-02 in time zone [id="Pacific/Enderbury",offset=46800000,dstSavings=0,useDaylight=false,transitions=5,lastRule=null]
toJavaDate(15338) = 2011-12-31 in time zone [id="Pacific/Fakaofo",offset=46800000,dstSavings=0,useDaylight=false,transitions=4,lastRule=null]
toJavaDate(9131) = 1995-01-02 in time zone [id="Pacific/Kiritimati",offset=50400000,dstSavings=0,useDaylight=false,transitions=5,lastRule=null]
toJavaDate(8632) = 1993-08-21 in time zone [id="Pacific/Kwajalein",offset=43200000,dstSavings=0,useDaylight=false,transitions=5,lastRule=null]
toJavaDate(15338) = 2011-12-31 in time zone [id="MIT",offset=46800000,dstSavings=3600000,useDaylight=true,transitions=59,lastRule=java.util.SimpleTimeZone[id=MIT,offset=46800000,dstSavings=3600000,useDaylight=true,startYear=0,startMode=2,startMonth=8,startDay=-1,startDayOfWeek=1,startTime=10800000,startTimeMode=0,endMode=3,endMonth=3,endDay=1,endDayOfWeek=1,endTime=14400000,endTimeMode=0]]

}
}
}
}