You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by omalley <gi...@git.apache.org> on 2018/02/27 04:56:58 UTC
[GitHub] orc pull request #220: ORC-306 Correct pre-1970 timestamps that were off by ...
GitHub user omalley opened a pull request:
https://github.com/apache/orc/pull/220
ORC-306 Correct pre-1970 timestamps that were off by one second.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/omalley/orc orc-306
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/orc/pull/220.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 #220
----
commit 1f38e1d58fe012dcf850ff1c0ba5d887ca0e21e4
Author: Owen O'Malley <om...@...>
Date: 2018-02-26T23:27:52Z
ORC-306 Correct pre-1970 timestamps that were off by one second.
----
---
[GitHub] orc pull request #220: ORC-306 Correct pre-1970 timestamps that were off by ...
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on a diff in the pull request:
https://github.com/apache/orc/pull/220#discussion_r171132343
--- Diff: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java ---
@@ -49,6 +50,15 @@
* Factory for creating ORC tree readers.
*/
public class TreeReaderFactory {
+ // The current JDK has a bug where values of the form:
+ // YYYY-MM-DD HH:MM:SS.000X is off by a second for dates before 1970.
+ public static final boolean TIMESTAMP_BUG;
+ static {
+ Timestamp ts1 = Timestamp.valueOf("1969-12-25 12:34:56.0001");
+ Timestamp ts2 = Timestamp.valueOf("1969-12-25 12:34:56.0011");
+ // Compare the seconds, which should be the same.
+ TIMESTAMP_BUG = ts1.getTime()/1000 != ts2.getTime()/1000;
--- End diff --
getTime() returns the milliseconds after 1970-01-01 00:00:00. So getTime()/1000 should return the seconds, which should be the same.
---
[GitHub] orc issue #220: ORC-306 Correct pre-1970 timestamps that were off by one sec...
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on the issue:
https://github.com/apache/orc/pull/220
So the fix detects whether the Timestamp class has the bug or not and then either compensates or doesn't. It also ensures that the TimestampColumnVector.time values are set correctly with the milliseconds on the bottom.
The test does more now. First of all, instead of going from 56.1000 to 56.1999 seconds incrementing by 0.0001, which doesn't reveal the bug, it goes from 56.0000 to 56.1999 seconds incrementing by 0.0002. It also checks the values from the TimestampColumnVector rather than comparing using Timestamp.
---
[GitHub] orc pull request #220: ORC-306 Correct pre-1970 timestamps that were off by ...
Posted by prasanthj <gi...@git.apache.org>.
Github user prasanthj commented on a diff in the pull request:
https://github.com/apache/orc/pull/220#discussion_r171117980
--- Diff: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java ---
@@ -49,6 +50,15 @@
* Factory for creating ORC tree readers.
*/
public class TreeReaderFactory {
+ // The current JDK has a bug where values of the form:
+ // YYYY-MM-DD HH:MM:SS.000X is off by a second for dates before 1970.
+ public static final boolean TIMESTAMP_BUG;
+ static {
+ Timestamp ts1 = Timestamp.valueOf("1969-12-25 12:34:56.0001");
+ Timestamp ts2 = Timestamp.valueOf("1969-12-25 12:34:56.0011");
+ // Compare the seconds, which should be the same.
+ TIMESTAMP_BUG = ts1.getTime()/1000 != ts2.getTime()/1000;
--- End diff --
I think in this case getSeconds() would return the same but millis would have changed as ts2 added a whole new millisecond to the time. isn't it?
---
[GitHub] orc issue #220: ORC-306 Correct pre-1970 timestamps that were off by one sec...
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on the issue:
https://github.com/apache/orc/pull/220
Here is another way to look at the problem with each timestamp's getTime() and getNanos():
Timestamp 1969-12-25 12:34:56.0 as -530704000 and 0
Timestamp 1969-12-25 12:34:56.0001 as -530704000 and 100000
Timestamp 1969-12-25 12:34:56.0011 as -530703999 and 1100000
When we use getTime()/1000 the change happens between .000999999 and .001000000.
If you don't think that Java will change, we could hard code the change to use 999_999 all of the time and not do the test.
---
[GitHub] orc pull request #220: ORC-306 Correct pre-1970 timestamps that were off by ...
Posted by prasanthj <gi...@git.apache.org>.
Github user prasanthj commented on a diff in the pull request:
https://github.com/apache/orc/pull/220#discussion_r171072157
--- Diff: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java ---
@@ -49,6 +50,15 @@
* Factory for creating ORC tree readers.
*/
public class TreeReaderFactory {
+ // The current JDK has a bug where values of the form:
+ // YYYY-MM-DD HH:MM:SS.000X is off by a second for dates before 1970.
--- End diff --
Can you please link the BUG # for future reference (can be done on commit)?
Also can we run the tests against JDK version with the bug on travis CI to trigger TIMSTAMP_BUG == true code path? The logic for adjusting millis look good to me.
---
[GitHub] orc pull request #220: ORC-306 Correct pre-1970 timestamps that were off by ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/orc/pull/220
---