ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568

Closed
julienledem wants to merge 6 commits into
apache:masterfrom
julienledem:ARROW-824
Closed

ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom
julienledem:ARROW-824

Conversation

@julienledem

@julienledemjulienledem commented Apr 19, 2017•
edited
Loading

Copy link
Copy Markdown
Member

The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.

@wesm

wesm commented Apr 19, 2017

Copy link
Copy Markdown
Member

I suppose failing the integration tests is good?

With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm

wesm commented Apr 21, 2017

Copy link
Copy Markdown
Member

@julienledem could you take a look at the integration test failure?

@julienledem

julienledem commented Apr 24, 2017

Copy link
Copy Markdown
MemberAuthor

@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.

@wesm

wesm commented Apr 24, 2017

Copy link
Copy Markdown
Member

Cool

@jacques-n

jacques-n commented May 1, 2017

Copy link
Copy Markdown
Contributor

Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.

@julienledem

julienledem commented May 1, 2017

Copy link
Copy Markdown
MemberAuthor

@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.

@jacques-n

jacques-n commented May 2, 2017

Copy link
Copy Markdown
Contributor

Got it, makes sense, thanks for the explanation @julienledem.

@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem

julienledem commented May 3, 2017

Copy link
Copy Markdown
MemberAuthor

This is good to go IMO

@wesm

wesm commented May 3, 2017

Copy link
Copy Markdown
Member

Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?

@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem

julienledem commented May 5, 2017

Copy link
Copy Markdown
MemberAuthor

@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).
It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.
To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.

@wesm

wesm commented May 6, 2017

Copy link
Copy Markdown
Member

Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?

@julienledem

julienledem commented May 6, 2017

Copy link
Copy Markdown
MemberAuthor

@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84

wesm
wesm approved these changes May 6, 2017
View reviewed changes

@wesmwesm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@asfgitasfgit closed this in 316c63dMay 6, 2017
@julienledemjulienledem mentioned this pull request May 8, 2017
Closed
jeffknupp pushed a commit to jeffknupp/arrow that referenced this pull request Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
ARROW-824: Date and Time Vectors should reflect timezone-less semantics - #568
Closed
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Closed
ARROW-824: Date and Time Vectors should reflect timezone-less semantics#568
julienledem wants to merge 6 commits into
apache:masterfrom 
julienledem:ARROW-824
Conversation
@julienledem
@julienledemjulienledem
commented
Apr 19, 2017•

edited

Loading

Copy link


Copy Markdown

Member
The current java vector support the timezone less version of time related types but in an incomplete way.

This change fixes it and clarifies what the vectors implementation do.

We'll need separate vectors or adapt those to deal will timezone aware time types.
@wesm
wesm
commented
Apr 19, 2017

Copy link


Copy Markdown

Member
I suppose failing the integration tests is good?
With output:
--------------
<SNIP>
22:26:05.180 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 10
22:26:05.184 [main] DEBUG o.a.a.vector.file.ArrowFileReader - Footer starts at 5992, length: 1064
22:26:05.184 [main] DEBUG o.a.arrow.vector.file.ReadChannel - Reading buffer with size: 1064
Incompatible files
only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
22:26:05.281 [main] ERROR org.apache.arrow.tools.Integration - Incompatible files
java.lang.IllegalArgumentException: only timezone-less timestamps are supported for now: Timestamp(MILLISECOND, America/New_York)
at org.apache.arrow.vector.types.Types$1.visit(Types.java:584) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types$1.visit(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.ArrowType$Timestamp.accept(ArrowType.java:929) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.Types.getMinorTypeForArrowType(Types.java:489) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.FieldType.createNewSingleVector(FieldType.java:56) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.types.pojo.Field.createVector(Field.java:89) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.initialize(ArrowReader.java:160) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.ensureInitialized(ArrowReader.java:143) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.vector.file.ArrowReader.getVectorSchemaRoot(ArrowReader.java:68) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration$Command$3.execute(Integration.java:171) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.run(Integration.java:101) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]
at org.apache.arrow.tools.Integration.main(Integration.java:62) ~[arrow-tools-0.2.1-SNAPSHOT-jar-with-dependencies.jar:na]

@wesm
wesm
commented
Apr 21, 2017

Copy link


Copy Markdown

Member
@julienledem could you take a look at the integration test failure?
@julienledem
julienledem
commented
Apr 24, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm Yes, failing the test is good. FYI: I removed this JIRA from the 0.3 release. I'll update the integration test.
@wesm
wesm
commented
Apr 24, 2017

Copy link


Copy Markdown

Member
Cool
@jacques-n
jacques-n
commented
May 1, 2017

Copy link


Copy Markdown

Contributor
Did you evaluate using the java8 apis instead of the joda ones? There is this backport for java 7: http://www.threeten.org/threetenbp/.
@julienledem
julienledem
commented
May 1, 2017

Copy link


Copy Markdown

MemberAuthor
@jacques-n Since those classes use a different package name, it does not look like there's a benefit to have them as an intermediate step. I'd rather move to the actual java 8 classes once arrow is on java 8. Moving to LocalDateTime is already a pretty big change without adding this on top.
@jacques-n
jacques-n
commented
May 2, 2017

Copy link


Copy Markdown

Contributor
Got it, makes sense, thanks for the explanation @julienledem.
@julienledem
improve travis integration
61518ec
Change-Id: I752bb6760c9ae3474e5af73593cd334872540d75
@julienledem
fix failing integration test
b0da88c
Change-Id: Ia375f6cf2164ee0c65582fc1b94483572192d565
@julienledem
make integration tests output more readable
bdac7ff
Change-Id: I3265d9ff676e7090d2d77b143c36a83fbf3f8e81
@julienledem
centralize LocalDateTime.toMillis
e37385c
Change-Id: I45026ec7f9a6afab6d6a63f18048872db5ac2e24
@julienledem
julienledem
commented
May 3, 2017

Copy link


Copy Markdown

MemberAuthor
This is good to go IMO
@wesm
wesm
commented
May 3, 2017

Copy link


Copy Markdown

Member
Removing the time zone feels like a regression. How difficult is it to preserve the time zone as metadata only?
@julienledem
add license
3528ad8
Change-Id: I1ae9518d401056143208206695a2c54fb94df3e1
@julienledem
julienledem
commented
May 5, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm The current vector implementation of the TimestampVector is TimezoneLess (except it was buggy because it used DateTime instead of LocalDateTime).

It worked in the integration tests because we never went through the Accessor.getObject/Reader/Writer java apis essentially just looking at the numeric timestamp value and ignoring the timezone.

To have a correct implementation we need a separate TZTimestampVector or a major refactor to decouple this logic from the vector.
@wesm
wesm
commented
May 6, 2017

Copy link


Copy Markdown

Member
Got it. I am ok with this, then. Let's open a JIRA to make a plan and implement something that works in time for 0.4 (end of this month)?
@julienledem
julienledem
commented
May 6, 2017

Copy link


Copy Markdown

MemberAuthor
@wesm OK: I have started the new vectors and some template cleanup here: 3e3ac84
wesm
wesm
approved these changes
May 6, 2017
View reviewed changes
@wesmwesm
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

+1
@asfgitasfgit
closed this
in
316c63dMay 6, 2017
@julienledemjulienledem
mentioned this pull request
May 8, 2017
 Closed

jeffknupp pushed a commit
to jeffknupp/arrow
that referenced
this pull request
Jun 3, 2017
@julienledem@jeffknupp
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
37eb76e
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
pribor pushed a commit
to GlobalWebIndex/arrow
that referenced
this pull request
Oct 24, 2025
@julienledem@kou
ARROW-824: Date and Time Vectors should reflect timezone-less semantics
e80f83f
The current java vector support the timezone less version of time related types but in an incomplete way.
This change fixes it and clarifies what the vectors implementation do.
We'll need separate vectors or adapt those to deal will timezone aware time types.
Author: Julien Le Dem <julien@apache.org>
Closesapache#568 from julienledem/ARROW-824 and squashes the following commits:
3528ad8 [Julien Le Dem] add license
e37385c [Julien Le Dem] centralize LocalDateTime.toMillis
bdac7ff [Julien Le Dem] make integration tests output more readable
b0da88c [Julien Le Dem] fix failing integration test
61518ec [Julien Le Dem] improve travis integration
ec19e7d [Julien Le Dem] ARROW-824: Date and Time Vectors should reflect timezone-less semantics
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@wesmwesmwesm approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@julienledem@wesm@jacques-n