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 3 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
39 changes: 36 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,29 @@ 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 = {
var guess = tz.getRawOffset
// the actual offset should be calculated based on milliseconds in UTC
var actual = tz.getOffset(millisLocal - guess)
if (actual != guess) {
// try with the result as guess
guess = actual
actual = tz.getOffset(millisLocal - guess)
// At the start of DST, the local time is forwarded by an hour, so it's OK to get the
// local time bigger than current one after an round trip (local -> UTC -> local).
// At the end of DST, the local time is backwarded by an hour, actual offset will be
// less than guess, we should decrease the guess and try again.
while (actual < guess) {
guess = actual
actual = tz.getOffset(millisLocal - guess)
}
}
guess
}

/**
* Returns a timestamp of given timezone from utc timestamp, with the same string
* representation in their timezone.
Expand All @@ -866,7 +889,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 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
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 number Diff line number Diff 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()
}
}
}
28 changes: 28 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,26 @@ 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") {
for (tz <- DateTimeTestUtils.ALL_TIMEZONES) {
DateTimeTestUtils.withDefaultTimeZone(tz) {
(-2000 to 2000).foreach { d =>
assert(millisToDays(daysToMillis(d)) === d)
}
Copy link
Member

@ckadner ckadner Jun 16, 2016 •
edited
Loading

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]]

}
}
}
}