[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/14398


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72878822
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +877,11 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val calendar = Calendar.getInstance(tz)
 
-// 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)
--- End diff --

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:
1) 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
2) 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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72877870
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +877,11 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val calendar = Calendar.getInstance(tz)
 
-// 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)
--- End diff --

no worries :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[ckadner]

Github user ckadner commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72877468
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +877,11 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val calendar = Calendar.getInstance(tz)
 
-// 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)
--- End diff --

I take that back -- I had my code tangled up -- sorry!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72877149
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +877,11 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val calendar = Calendar.getInstance(tz)
 
-// 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)
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[ckadner]

Github user ckadner commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72876907
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +877,11 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val calendar = Calendar.getInstance(tz)
 
-// 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)
--- End diff --

`   calendar.set(year, month - 1, day, hh - 1, mm, ss)`  makes the 
existing test cases work


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72845861
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +878,20 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val millis = millisOfDay % 1000
+// Choose calendar based on java.util.Date getCalendarSystem
+val calendar = if (year >= 1582) {
+  CalendarSystem.getGregorianCalendar()
--- End diff --

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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72845540
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -24,6 +24,8 @@ import javax.xml.bind.DatatypeConverter
 
 import scala.annotation.tailrec
 
+import sun.util.calendar.CalendarSystem
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[holdenk]

Github user holdenk commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72844129
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +878,20 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val millis = millisOfDay % 1000
+// Choose calendar based on java.util.Date getCalendarSystem
+val calendar = if (year >= 1582) {
+  CalendarSystem.getGregorianCalendar()
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72789569
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -875,11 +878,20 @@ object DateTimeUtils {
 val hh = seconds / 3600
 val mm = seconds / 60 % 60
 val ss = seconds % 60
-val nano = millisOfDay % 1000 * 100
+val millis = millisOfDay % 1000
+// Choose calendar based on java.util.Date getCalendarSystem
+val calendar = if (year >= 1582) {
+  CalendarSystem.getGregorianCalendar()
--- End diff --

I think we can safely assume the Gregorian calendar? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/14398#discussion_r72789531
  
--- Diff: 
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
 ---
@@ -24,6 +24,8 @@ import javax.xml.bind.DatatypeConverter
 
 import scala.annotation.tailrec
 
+import sun.util.calendar.CalendarSystem
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #14398: [SPARK-16774][SQL] Fix use of deprecated timestam...

[holdenk]

GitHub user holdenk opened a pull request:

https://github.com/apache/spark/pull/14398

[SPARK-16774][SQL] Fix use of deprecated timestamp constructor & improve 
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.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/holdenk/spark 
SPARK-16774-fix-use-of-deprecated-timestamp-constructor

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/14398.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #14398


commit 7c0fd8d4f50378b419bfd0059fe31f6e95205fb3
Author: Holden Karau 
Date:   2016-07-28T04:23:00Z

Stop using deprecated Timestamp constructor

commit 634a05e3c7afed2987efdcb0680b2e3547ae10a2
Author: Holden Karau 
Date:   2016-07-29T00:25:45Z

Wait the old timezone code was badnews bears - did stuff based implicitly 
on local system timezone - lets avoid that

commit 6233b4cbdc55930334634b9acadea627117e6e23
Author: Holden Karau 
Date:   2016-07-29T05:55:13Z

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

commit e4508e32662f9eba4621c7ffb54ec7661dbc54fd
Author: Holden Karau 
Date:   2016-07-29T06:01:21Z

not used anymore

commit 0385d8bd3f7ea85abb14191a4f3eb7eb1dbd4582
Author: Holden Karau 
Date:   2016-07-29T06:07:37Z

Just remove the roundtrip test for now - can be added back from git history 
later




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org