ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457

Closed
rok wants to merge 10 commits into
apache:masterfrom
rok:ARROW-12980
Closed

ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom
rok:ARROW-12980

Conversation

@rok

@rokrok commented Jun 4, 2021

Copy link
Copy Markdown
Member

@github-actions

github-actionsBot commented Jun 4, 2021

Copy link
Copy Markdown

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@rokrok changed the title [ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions

github-actionsBot commented Jun 4, 2021

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980

@rokrok mentioned this pull request Jun 4, 2021
Closed
@rok
rokforce-pushed the ARROW-12980 branch 2 times, most recently from f6b2ddf to 7dcdb9bCompareJune 6, 2021 02:01
@rok

rok commented Jun 6, 2021

Copy link
Copy Markdown
MemberAuthor

Errors appear to be due to windows builds not finding tz database. See.

@rok
rokforce-pushed the ARROW-12980 branch 2 times, most recently from 7975c67 to c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche

jorisvandenbossche commented Jun 10, 2021•
edited
Loading

Copy link
Copy Markdown
Member

I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).
(while in python we could create the localized input timestamps with pandas)

Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.

@rok

rok commented Jun 10, 2021

Copy link
Copy Markdown
MemberAuthor

I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).
(while in python we could create the localized input timestamps with pandas)

Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.
It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..

Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.

Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.

Also strptime kernel ignores timezones at the moment.

@jorisvandenbossche

jorisvandenbossche commented Jun 10, 2021

Copy link
Copy Markdown
Member

Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?

I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.

I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.

@jorisvandenbossche

jorisvandenbossche commented Jun 10, 2021

Copy link
Copy Markdown
Member

Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?

The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?

@rok

rok commented Jun 10, 2021

Copy link
Copy Markdown
MemberAuthor

I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.

Nice, I was also writing it just now :)
The strptime timzone ignoring issue is here.

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?

As per HowardHinnant's answer:

The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.

I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).

@rok

rok commented Jun 10, 2021

Copy link
Copy Markdown
MemberAuthor

@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).

@rok
rokforce-pushed the ARROW-12980 branch 2 times, most recently from d460b7f to 62a7915CompareJune 10, 2021 18:02
@rok

rok commented Jun 10, 2021

Copy link
Copy Markdown
MemberAuthor

Added the timezone-naive test for component extractions.
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.

jorisvandenbossche
jorisvandenbossche reviewed Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py Outdated
Comment threadpython/pyarrow/tests/test_compute.py Outdated
@jorisvandenbossche

jorisvandenbossche commented Jun 14, 2021

Copy link
Copy Markdown
Member

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.

I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.

@rok
rokforce-pushed the ARROW-12980 branch 5 times, most recently from 7bcd926 to 60c0d4dCompareJune 14, 2021 21:21
@rok

rok commented Jun 14, 2021

Copy link
Copy Markdown
MemberAuthor

I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.

Huh, I forget exactly what I was thinking there and you appear to be right.

I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.

rok
rok commented Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
Comment threadpython/pyarrow/tests/test_compute.py Outdated
@rok
rokforce-pushed the ARROW-12980 branch 2 times, most recently from db1ecb2 to 78269adCompareJuly 27, 2021 17:22
@rokrok mentioned this pull request Jul 28, 2021
Closed
pitrou
pitrou requested changes Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.

@rokrokAug 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.

@rokrokAug 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

Comment threaddocs/source/cpp/compute.rst Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".

@rokrokAug 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done.

Comment threaddocs/source/cpp/compute.rst Outdated
@rok
rokforce-pushed the ARROW-12980 branch 3 times, most recently from bb23f0a to 869c62aCompareAugust 13, 2021 02:40
@rok

rok commented Aug 13, 2021

Copy link
Copy Markdown
MemberAuthor

@pitrou Thanks for the review! And sorry for late reply.
Could you please review again and suggest if templating could be further improved.

jorisvandenbossche
jorisvandenbossche reviewed Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?

@rokrokAug 13, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Renamed the test.

@lidavidmlidavidm mentioned this pull request Aug 13, 2021
Closed
pitrou
pitrou reviewed Aug 16, 2021
View reviewed changes

@pitroupitrou 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.

A couple more comments. I'll do the changes myself.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated

@pitroupitrouAug 16, 2021•
edited
Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...

@rokrokAug 16, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.

@rok

rok commented Aug 16, 2021

Copy link
Copy Markdown
MemberAuthor

@pitrou thanks for the review and the changes - those look much better!

@pitrou

pitrou commented Aug 16, 2021

Copy link
Copy Markdown
Member

Rebased again to pick up AppVeyor fix :-/

@rok

rok commented Aug 16, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks @pitrou@jorisvandenbossche and @kszucs !

nealrichardson
nealrichardson reviewed Aug 16, 2021
View reviewed changes

@nealrichardsonnealrichardson 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.

One question on the R changes in this PR

Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {

@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.

@rokrokAug 16, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.
A comment here could state:

# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?

@rokrokAug 17, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.

Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees

No one assigned

Labels

Component: C++Component: PythonComponent: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} 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-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson

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

Skip to content
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson

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

Skip to content
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} 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-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson

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

Skip to content
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson

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

Skip to content
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson

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

Skip to content
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware - #10457
Closed
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Closed
ARROW-12980: [C++] Kernels to extract datetime components should be timezone aware#10457
rok wants to merge 10 commits into
apache:masterfrom 
rok:ARROW-12980
Conversation
@rok
@rokrok
commented
Jun 4, 2021

Copy link


Copy Markdown

Member
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

Thanks for opening a pull request!
If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or
MINOR: [${COMPONENT}] ${SUMMARY}

See also:
@rokrok
changed the title
[ARROW-12980ARROW-12980: [C++] Kernels to extract datetime components should be timezone awareJun 4, 2021
@github-actions
github-actionsBot
commented
Jun 4, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-12980
@rokrok
mentioned this pull request
Jun 4, 2021
 Closed

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
f6b2ddf to
7dcdb9bCompareJune 6, 2021 02:01
@rok
rok
commented
Jun 6, 2021

Copy link


Copy Markdown

MemberAuthor
Errors appear to be due to windows builds not finding tz database. See.
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
7975c67 to
c0b0bc7CompareJune 9, 2021 18:09
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021•

edited

Loading

Copy link


Copy Markdown

Member
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I think it would be good to write some tests in python as well, as currently the C++ tests are very hard to verify since we don't yet have the ability to parse strings localized in the timezone (I mean: the strings in the tests are interpreted as UTC and not the "Australia/Broken_Hill" timezone. And thus as a result, the expected values can't be read/verified from the strings).

(while in python we could create the localized input timestamps with pandas)
Indeed! I've used that approach to generate data in ARROW-11759 by pandas already and I'll just adopt that code here.

It did lead me to an odd issue? in pandas that I cant quite explain. We'll either need to address it or avoid it in tests ..
Given that, it might also make sense to first add a "localize" kernel for converting timestamps from naive to a certain timezone.
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not? Vector version would apply here.
Also strptime kernel ignores timezones at the moment.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Localize kernel would be great. I suppose we'd need a scalar and a vector one depending if timezone is shared between rows or not?
I think a scalar kernel is the most important (with signature like localize(array[timestamp], string tz) -> array[timestamp, tz], where array can also be chunked array or scalar of course). Mixed timezones is a rather corner case (and in principle also could be done as a scalar kernel if the second tz argument is also an array). I think ARROW-5912 is more a bug in the python->arrow conversion code, and not necessarily related / fixable by a localize kernel.
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Another question: is the locate_zone configurable in some way to give some hints where to find the tz database?
The database is not always available on the system (as you noticed for windows). But depending on the application / binding language, it might already be available elsewhere. For example, in recent Python versions, there is now support for this in the standard library, automatically using the tzdata package if there is no system database. I am wondering if pyarrow should be able to instruct the kernel where to look for the database (https://www.python.org/dev/peps/pep-0615/#search-path-configuration in python).

Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
I opened https://issues.apache.org/jira/browse/ARROW-13033 for this.
Nice, I was also writing it just now :)

The strptime timzone ignoring issue is here.
Or does the vendored date.h also include functionalities to automatically download the data if not available on the system?
As per HowardHinnant's answer:
The library can be configured to not go to the internet for tzdata. tzdata can either be downloaded manually, and this lib can find it, or (on non-Windows systems), the OS-supplied tzdata can be used.
I suppose we could have the kernel try getting data online. If that fails try OS and as a final fallback use arrow bundled tz db (which we would have to add in this PR).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
@jorisvandenbossche added the python tests. Will add some more for timezone naive and if needed make another PR :).
@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
d460b7f to
62a7915CompareJune 10, 2021 18:02
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Added the timezone-naive test for component extractions.

I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 14, 2021
View reviewed changes
Comment threadpython/pyarrow/tests/test_compute.py
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@jorisvandenbossche
jorisvandenbossche
commented
Jun 14, 2021

Copy link


Copy Markdown

Member
I'll wait for the consensus to build on the timezone handling discussions before closing the PR and moving the python tests to a new PR.
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
@rok
rokforce-pushed
the
ARROW-12980
branch
5 times, most recently
from
7bcd926 to
60c0d4dCompareJune 14, 2021 21:21
@rok
rok
commented
Jun 14, 2021

Copy link


Copy Markdown

MemberAuthor
I think there was no opposition for this one (i.e. that the field extraction should yield local hour/minute/etc), so I don't think there is a need to close the PR.
Huh, I forget exactly what I was thinking there and you appear to be right.
I think the main remaining task is the tz db issue. I'll take a look at it tomorrow.
rok
rok
commented
Jun 14, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadpython/pyarrow/tests/test_compute.py
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
2 times, most recently
from
db1ecb2 to
78269adCompareJuly 27, 2021 17:22
@rokrok
mentioned this pull request
Jul 28, 2021
 Closed

pitrou
pitrou
requested changes
Aug 2, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@pitroupitrouAug 2, 2021

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.

You should move this up so that it only encloses tests that don't work on Windows.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 2, 2021

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.

I'm sure you can find a way to share this with MakeTemporal below.
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@pitroupitrouAug 2, 2021

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.

Rewrite this as (for example): "If the input timestamps have a non-empty timezone, localized timestamp components will be returned".
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Done.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-12980
branch
3 times, most recently
from
bb23f0a to
869c62aCompareAugust 13, 2021 02:40
@rok
rok
commented
Aug 13, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou Thanks for the review! And sorry for late reply.

Could you please review again and suggest if templating could be further improved.
jorisvandenbossche
jorisvandenbossche
reviewed
Aug 13, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

@jorisvandenbosschejorisvandenbosscheAug 13, 2021

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.

Can you add a comment here (or embed it in the name of the test) that this is a test for a invalid / non-existing timezone?
@rokrokAug 13, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Renamed the test.
@lidavidmlidavidm
mentioned this pull request
Aug 13, 2021
 Closed

pitrou
pitrou
reviewed
Aug 16, 2021
View reviewed changes
@pitroupitrou
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.

A couple more comments. I'll do the changes myself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

You can't take a const-reference to a temporary dynamically created value.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021•

edited

Loading

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.

Can probably avoid the overhead of std::function by templating with the callable type itself.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

@pitroupitrouAug 16, 2021

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.

At this point it should be possible to reconcile this with TemporalComponentExtract above...
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Indeed. This will be useful for the Strftime and localize kernels as well.
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
@pitrou thanks for the review and the changes - those look much better!
@pitrou
pitrou
commented
Aug 16, 2021

Copy link


Copy Markdown

Member
Rebased again to pick up AppVeyor fix :-/
@rok
rok
commented
Aug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @pitrou@jorisvandenbossche and @kszucs !
nealrichardson
nealrichardson
reviewed
Aug 16, 2021
View reviewed changes
@nealrichardsonnealrichardson
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.

One question on the R changes in this PR
Comment threadr/tests/testthat/test-dplyr-lubridate.R
withr::local_timezone("UTC")

test_date <- as.POSIXct("2017-01-01 00:00:12.3456789", tz = "")
if (tolower(Sys.info()[["sysname"]]) == "windows") {
@nealrichardsonnealrichardsonAug 16, 2021

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.

Why is windows special-cased here? This needs an explanatory comment and possibly a link to a jira that will let us delete this.
@rokrokAug 16, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

This is due to the timezone db issue on windows.

A comment here could state:
# TODO: We should test on windows once ARROW-13168 is resolved.

@nealrichardsonnealrichardsonAug 17, 2021

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.

If this doesn't work on windows, why did we have to add -lole32 to r/configure.win in this patch?
@rokrokAug 17, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Without -lole32 flag all zoned time logic was causing errors at compile time (if I remember correctly) so it would have to be disabled for windows. Since we will eventually have zones on windows I went ahead and rather added the flag.
Here's a similar issue from date.h and a similar discussion from another PR.

This was referenced Aug 17, 2021

Closed


Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@nealrichardsonnealrichardsonnealrichardson left review comments
@pitroupitroupitrou approved these changes
@kszucskszucsAwaiting requested review from kszucs

Assignees


No one assigned


Labels

Component: C++Component: PythonComponent: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@rok@jorisvandenbossche@kszucs@pitrou@nealrichardson