Skip to content

[SPARK-16774][SQL] Fix use of deprecated timestamp constructor & improve timezone handling #14398

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
holdenk wants to merge 12 commits into from
Closed

[SPARK-16774][SQL] Fix use of deprecated timestamp constructor & improve timezone handling #14398

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7c0fd8d
Stop using deprecated Timestamp constructor
holdenk Jul 28, 2016
634a05e
Wait the old timezone code was badnews bears - did stuff based implic…
holdenk Jul 29, 2016
6233b4c
Simplify & disable the roundtrip test - it wasn't really running anyw…
holdenk Jul 29, 2016
e4508e3
not used anymore
holdenk Jul 29, 2016
0385d8b
Just remove the roundtrip test for now - can be added back from git h…
holdenk Jul 29, 2016
5e80a8d
Use standard calendar
holdenk Jul 29, 2016
c01db02
Don't need to capture return value of set
holdenk Jul 29, 2016
ec75f73
Add another TODO with JIRA link
holdenk Jul 29, 2016
7c28252
Merge branch 'master' into SPARK-16774-fix-use-of-deprecated-timestam…
holdenk Jul 29, 2016
8670bb6
Add back tests and add some more notes
holdenk Jul 30, 2016
1109275
nvm about those localmilis based tests - should be covered through to…
holdenk Jul 30, 2016
671f7be
Remove superflous getTime call
holdenk Jul 31, 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
22 changes: 17 additions & 5 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 @@ -24,6 +24,8 @@ import javax.xml.bind.DatatypeConverter

import scala.annotation.tailrec

import sun.util.calendar.CalendarSystem
Copy link
Member

@srowen srowen Jul 29, 2016

Choose a reason for hiding this comment

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

We should be using java.util.Calendar right -- I presume any sun. classes are not for use by applications.

Copy link
Contributor Author

@holdenk holdenk Jul 29, 2016

Choose a reason for hiding this comment

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

Seems reasonable - will also allow us to hide the julian calendar usage for old dates.


import org.apache.spark.unsafe.types.UTF8String

/**
Expand Down Expand Up @@ -852,8 +854,9 @@ object DateTimeUtils {

/**
* Lookup the offset for given millis seconds since 1970-01-01 00:00:00 in given timezone.
* TODO: Improve handling of normalization differences.
*/
private def getOffsetFromLocalMillis(millisLocal: Long, tz: TimeZone): Long = {
private[sql] 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)
Expand All @@ -875,11 +878,20 @@ object DateTimeUtils {
val hh = seconds / 3600
val mm = seconds / 60 % 60
val ss = seconds % 60
val nano = millisOfDay % 1000 * 1000000
val millis = millisOfDay % 1000
// Choose calendar based on java.util.Date getCalendarSystem
val calendar = if (year >= 1582) {
CalendarSystem.getGregorianCalendar()
Copy link
Member

@srowen srowen Jul 29, 2016

Choose a reason for hiding this comment

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

I think we can safely assume the Gregorian calendar?

Copy link
Contributor Author

@holdenk holdenk Jul 29, 2016

Choose a reason for hiding this comment

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

I hope so? But if someone has weird historic data I'd rather give it a shot.

Copy link
Member

@srowen srowen Jul 29, 2016

Choose a reason for hiding this comment

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

For this to happen millisLocal would have to be negative... which is at least well defined, but, would certainly need to be defined relative to January 1 1970 in the Gregorian calendar?

} else {
CalendarSystem.forName("julian")
}

// 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
// create a CalendarDate in the provided timezone
val date = calendar.newCalendarDate(tz).
setDate(year, month, day).
setTimeOfDay(hh, mm, ss, millis)
calendar.getTime(date) // Set the timezone info
guess = date.getZoneOffset()
}
}
guess
Expand Down
50 changes: 30 additions & 20 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 @@ -536,25 +536,35 @@ class DateTimeUtilsSuite extends SparkFunSuite {
}
}

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)
}
}
}
}
test("convert TZ with boundary time pacific") {
val tz = TimeZone.getTimeZone("America/Los_Angeles")
val boundaryDates = List(
1425680000000L, // March 6, 2015 @ 10:13:20 pm
1425779000000L, // March 8, 2015 @ 1:43:20 am
1425780000000L, // March 8, 2015 @ 3:00:00 am
1425781000000L, // March 8, 2015 @ 3:16:40 am
1425783600000L, // March 8, 2015 @ 4:00:00 am
1425880000000L, // March 9, 2015 @ 6:46:40 am
1004580000000L, // November 1, 2001 @ 1:00:00 am
1004580060000L // November 1, 2001 @ 1:01:00 am
)
val offsets = boundaryDates.map(boundaryDate =>
DateTimeUtils.getOffsetFromLocalMillis(boundaryDate, tz))
assert(
List(-28800000, -28800000, -25200000, -25200000, -25200000, -25200000, -28800000, -28800000)
=== offsets)
}

test("convert TZ with boundary time London") {
val tz = TimeZone.getTimeZone("Europe/London")
val boundaryDates = List(
1459040340000L, // March 27, 2016 @ 12:59:00 am
1459040400000L, // March 27, 2016 @ 2:00:00 am
1459040410000L, // March 27, 2016 @ 2:16:40 am
1459126800000L // March 28, 2016 @ 2:00:00 am
)
val offsets = boundaryDates.map(boundaryDate =>
DateTimeUtils.getOffsetFromLocalMillis(boundaryDate, tz))
assert(List(0, 3600000, 3600000, 3600000) == offsets)
}
}