[SPARK-16774][SQL] Fix use of deprecated timestamp constructor & improve timezone handling by holdenk · Pull Request #14398 · apache/spark

holdenk · 2016-07-29T06:10:38Z

What changes were proposed in this pull request?

Removes the deprecated timestamp constructor and incidentally fixes the use which was using system timezone rather than the one specified when working near DST.

This change also causes the roundtrip tests to fail since it now actually uses all the timezones near DST boundaries where it didn't before.

Note: this is only a partial the solution, longer term we should follow up with https://issues.apache.org/jira/browse/SPARK-16788 to avoid this problem & simplify our timezone handling code.

How was this patch tested?

New tests for two timezones added so even if user timezone happens to coincided with one, the other tests should still fail. Important note: this (temporarily) disables the round trip tests until we can fix the issue more thoroughly.

…itly on local system timezone - lets avoid that

…ays since it was always falling back to system timezone in the fall through case

…istory later

holdenk · 2016-07-29T06:28:42Z

cc @ckadner & @davies - still think the longterm solution is a larger rewrite of this into a thin wrapper around something like JSR-310, but in the meantime this should be some progress.

SparkQA · 2016-07-29T08:00:25Z

Test build #62997 has finished for PR 14398 at commit 0385d8b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-07-29T13:19:47Z


 import scala.annotation.tailrec

+import sun.util.calendar.CalendarSystem


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

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

holdenk · 2016-07-29T19:00:02Z

Also http://infiniteundo.com/post/25509354022/more-falsehoods-programmers-believe-about-time - I really think we should stop handling time as much as possibly and leave it to people who spend enough time thinking about it to get it mostly right. :p

ckadner · 2016-07-29T22:30:30Z

@holdenk @davies
I agree that date, time, and timezone handling is best left to the experts and leveraging JSR-310 (SPARK-16788) in the long run would probably solve most of the problems that have been painstakingly fixed or attempted to be fixed in DateTimeUtils.scala.

The change proposed in this PR makes for cleaner code but it also regresses the fix for SPARK-15613. With this fix there are 4 incorrect day -> millis -> day conversions since 1970/01/01 just for Europe/Moscow:

 test("15613 Incorrect days to millis conversion Europe/Moscow") {
DateTimeTestUtils.withDefaultTimeZone(TimeZone.getTimeZone("Europe/Moscow")) {
valbadDays= (0 to 20000).filterNot{ d =>
d === millisToDays(daysToMillis(d))
}
valdf=newSimpleDateFormat("yyyy-MM-dd HH:mm:ss z")
badDays.foreach { d =>
println(s"day: ${d}, date: ${df.format(newDate(daysToMillis(d)))}")
}
assert(badDays.isEmpty)
}
}

TestFailedException: Vector(4108, 4473, 4838, 5204) was not emptyday: 4108, date: 1981-03-31 23:00:00 MSKday: 4473, date: 1982-03-31 23:00:00 MSKday: 4838, date: 1983-03-31 23:00:00 MSKday: 5204, date: 1984-03-31 23:00:00 MSK

...and it increases the number of incorrectly converted days from 52 to 4036 when testing all time zones and dates between 1900 and 2020 (see test code snippet here).

…p-constructor

ckadner · 2016-07-30T00:13:12Z

-// 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
+calendar.set(year, month, day, hh, mm, ss)


calendar.set(year, month - 1, day, hh, mm, ss) and
calendar.clear(Calendar.MILLISECOND) and
guess = (millisLocal - calendar.getTimeInMillis).toInt
makes the existing test cases work
val calendar = Calendar.getInstance(tz) calendar.set(year, month - 1, day, hh, mm, ss) calendar.clear(Calendar.MILLISECOND) guess = (millisLocal - calendar.getTimeInMillis).toInt

huh one would think that the tests with the timestamps close to DST looking up offsets would also need this if that was the intended constructor but ok let me give it a shot.

no worries :)

ok so my (previous) comments don't make sense in response to the comment edit but I'll leave as is.
As for the newly updated comment, the approach does avoid the deprecation warning while keeping the existing functionality - but:
Its going to throw away any subsecond information in the timestamp by including it in the difference from millis local to the time in miliseconds
It doesn't seem to resolve the timezone offset correctly (it seems to give back non-DST times in DST transitions)*
That being said - it is a strict improvement over the previous code so we could go for this approach instead since it is only a stop-gap solution and its really about choosing which way we want this code to be more broken in.

So DST is probably ok and my tests are just borked - so lets go with the new calendar approach and I'll fix it to preserve ms information.

… UTC tests

SparkQA · 2016-07-30T01:49:07Z

Test build #63030 has finished for PR 14398 at commit 1109275.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

ckadner · 2016-07-30T02:03:46Z

+ val calendar = Calendar.getInstance(tz)
+ calendar.set(year, month - 1, day, hh, mm, ss)
+ calendar.set(Calendar.MILLISECOND, ms)
+ val date = calendar.getTime()


this seems unnecessary ... if the intend is to make sure Calendar.computeTime() will be invoked then that will happen when calling calendar.getTimeInMillis() on the next line as well

SparkQA · 2016-07-30T02:38:33Z

Test build #63028 has finished for PR 14398 at commit 8670bb6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-31T02:36:55Z

Test build #63050 has finished for PR 14398 at commit 671f7be.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2016-07-31T23:51:12Z

Just for good measure lets re-run the jenkins tests.
Jenkins retest this please.

srowen · 2016-08-01T14:09:20Z

Jenkins retest this please

srowen · 2016-08-01T14:09:27Z

The approach LGTM

SparkQA · 2016-08-01T16:13:45Z

Test build #63078 has finished for PR 14398 at commit 671f7be.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2016-08-01T19:08:18Z

Yay - I still think we want to replace this entire file with a thin wrapper around specialized library at some point soon but incremental progress (avoid warning - don't depend on system timezone randomly near DST boundaries during timestamp conversion - which I think might be related to the issue Reynold added some logging for recently).

ckadner · 2016-08-01T19:25:47Z

LGTM

srowen · 2016-08-01T20:57:32Z

Merged to master/2.0

…ove timezone handling ## What changes were proposed in this pull request? Removes the deprecated timestamp constructor and incidentally fixes the use which was using system timezone rather than the one specified when working near DST. This change also causes the roundtrip tests to fail since it now actually uses all the timezones near DST boundaries where it didn't before. Note: this is only a partial the solution, longer term we should follow up with https://issues.apache.org/jira/browse/SPARK-16788 to avoid this problem & simplify our timezone handling code. ## How was this patch tested? New tests for two timezones added so even if user timezone happens to coincided with one, the other tests should still fail. Important note: this (temporarily) disables the round trip tests until we can fix the issue more thoroughly. Author: Holden Karau <holden@us.ibm.com> Closes#14398 from holdenk/SPARK-16774-fix-use-of-deprecated-timestamp-constructor. (cherry picked from commit ab1e761) Signed-off-by: Sean Owen <sowen@cloudera.com>

holdenk added 5 commits July 28, 2016 11:26

Stop using deprecated Timestamp constructor
7c0fd8d

Wait the old timezone code was badnews bears - did stuff based implic…
634a05e
…itly on local system timezone - lets avoid that

Simplify & disable the roundtrip test - it wasn't really running anyw…
6233b4c
…ays since it was always falling back to system timezone in the fall through case

not used anymore
e4508e3

Just remove the roundtrip test for now - can be added back from git h…
0385d8b
…istory later

srowen reviewed Jul 29, 2016
View reviewed changes

holdenk added 3 commits July 29, 2016 12:30

Use standard calendar
5e80a8d

Don't need to capture return value of set
c01db02

Add another TODO with JIRA link
ec75f73

Merge branch 'master' into SPARK-16774-fix-use-of-deprecated-timestam…
7c28252
…p-constructor

ckadner reviewed Jul 30, 2016
View reviewed changes

Add back tests and add some more notes
8670bb6

holdenk changed the title ~~[SPARK-16774][SQL] Fix use of deprecated timestamp constructor & improve timezone handling~~[SPARK-16774][SQL][WIP] Fix use of deprecated timestamp constructor & improve timezone handlingJul 30, 2016

nvm about those localmilis based tests - should be covered through to…
1109275
… UTC tests

ckadner reviewed Jul 30, 2016
View reviewed changes

Remove superflous getTime call
671f7be

holdenk changed the title ~~[SPARK-16774][SQL][WIP] Fix use of deprecated timestamp constructor & improve timezone handling~~[SPARK-16774][SQL] Fix use of deprecated timestamp constructor & improve timezone handlingAug 1, 2016

asfgit closed this in ab1e761Aug 1, 2016


		import scala.annotation.tailrec

		import sun.util.calendar.CalendarSystem

Uh oh!

Conversation

holdenk commented Jul 29, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

holdenk commented Jul 29, 2016

Uh oh!

SparkQA commented Jul 29, 2016

Uh oh!

srowenJul 29, 2016

Choose a reason for hiding this comment

Uh oh!

holdenkJul 29, 2016

Choose a reason for hiding this comment

Uh oh!

holdenk commented Jul 29, 2016

Uh oh!

ckadner commented Jul 29, 2016

Uh oh!

ckadnerJul 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holdenkJul 30, 2016

Choose a reason for hiding this comment

Uh oh!

holdenkJul 30, 2016

Choose a reason for hiding this comment

Uh oh!

holdenkJul 30, 2016

Choose a reason for hiding this comment

Uh oh!

holdenkJul 30, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 30, 2016

Uh oh!

ckadnerJul 30, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 30, 2016

Uh oh!

SparkQA commented Jul 31, 2016

Uh oh!

holdenk commented Jul 31, 2016

Uh oh!

srowen commented Aug 1, 2016

Uh oh!

srowen commented Aug 1, 2016

Uh oh!

SparkQA commented Aug 1, 2016

Uh oh!

holdenk commented Aug 1, 2016

Uh oh!

ckadner commented Aug 1, 2016

Uh oh!

srowen commented Aug 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ckadnerJul 30, 2016•
edited
Loading