ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176

Closed
rok wants to merge 17 commits into
apache:masterfrom
rok:ARROW-11759
Closed

ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom
rok:ARROW-11759

Conversation

@rok

@rokrok commented Apr 27, 2021

Copy link
Copy Markdown
Member

This is to resolve ARROW-11759.

@github-actions

github-actionsBot commented Apr 27, 2021

Copy link
Copy Markdown

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

@rok

rok commented Apr 27, 2021

Copy link
Copy Markdown
MemberAuthor

Missing features at moment:

  • isocalendar
  • millisecond
  • microsecond
  • nanosecond

Different input / output types should also be handled.
Does this need to be timezone aware?

@jorisvandenbossche

jorisvandenbossche commented Apr 28, 2021

Copy link
Copy Markdown
Member

Does this need to be timezone aware?

Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?

@rok
rokforce-pushed the ARROW-11759 branch 4 times, most recently from 2c4968d to 2765cefCompareMay 6, 2021 16:20
@rok

rok commented May 6, 2021

Copy link
Copy Markdown
MemberAuthor

It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.

Open questions I have at the moment:

  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?

@rok
rok marked this pull request as ready for review May 6, 2021 17:22
@rok

rok commented May 8, 2021

Copy link
Copy Markdown
MemberAuthor

I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).

pitrou
pitrou requested changes May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc Outdated
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
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
@jorisvandenbossche

jorisvandenbossche commented May 10, 2021

Copy link
Copy Markdown
Member

General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)

@rok

rok commented May 12, 2021

Copy link
Copy Markdown
MemberAuthor

General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)

Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.

@rok
rokforce-pushed the ARROW-11759 branch 8 times, most recently from f8c6665 to 23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche reviewed May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
jorisvandenbossche
jorisvandenbossche reviewed May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h Outdated
@rok
rokforce-pushed the ARROW-11759 branch 3 times, most recently from 7bcc910 to 69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche reviewed Jun 9, 2021
View reviewed changes

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

Thanks for the updates! Did a final pass through the docs.

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc Outdated
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
{"values"}};

const FunctionDoc millisecond_doc{
"Extract millisecond values",

@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)

@rokrokJun 9, 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.cc
{"values"}};

const FunctionDoc subsecond_doc{
"Extract subsecond values",

@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring

@rokrokJun 9, 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.cc
}
return {static_cast<int64_t>(static_cast<int32_t>(y)),
static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
static_cast<int64_t>(weekday(ymd).iso_encoding())};

@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?

@rokrokJun 9, 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.

Added to docs and prepended.

Comment threaddocs/source/cpp/compute.rst Outdated
@rok
rokforce-pushed the ARROW-11759 branch 2 times, most recently from 4dc50cb to ee53452CompareJune 9, 2021 12:03
@rok

rok commented Jun 9, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks for the comments and suggestions @jorisvandenbossche! :)

pitrou
pitrou reviewed Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst Outdated
@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou

pitrou commented Jun 9, 2021

Copy link
Copy Markdown
Member

Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.

jorisvandenbossche
jorisvandenbossche reviewed Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst Outdated
@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok

rok commented Jun 9, 2021

Copy link
Copy Markdown
MemberAuthor

Just noticed I missed this:

@jorisvandenbossche
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?

Do you mind if I move it to ARROW-12980?

@rok

rok commented Jun 9, 2021

Copy link
Copy Markdown
MemberAuthor

Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?
Like ` "1970-01-01T00:00:59.1234567890123"?

@pitroupitrou closed this in 00cc41fJun 9, 2021
@rok
rok deleted the ARROW-11759 branch June 9, 2021 19:04
@jorisvandenbossche

jorisvandenbossche commented Jun 10, 2021

Copy link
Copy Markdown
Member

Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)

what do you mean by "doesn't fit into the nanosecond range"?

I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).

@rok

rok commented Jun 10, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)

@rokrok mentioned this pull request Jun 10, 2021
Closed
@asfimportasfimport mentioned this pull request Aug 4, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou
, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou

, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou

, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou

, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou

, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou

, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou

, '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-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type - #10176
Closed
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Closed
ARROW-11759: [C++] Kernel to extract datetime components (year, month, day, etc) from timestamp type#10176
rok wants to merge 17 commits into
apache:masterfrom 
rok:ARROW-11759
Conversation
@rok
@rokrok
commented
Apr 27, 2021

Copy link


Copy Markdown

Member
This is to resolve ARROW-11759.
@github-actions
github-actionsBot
commented
Apr 27, 2021

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-11759
@rok
rok
commented
Apr 27, 2021

Copy link


Copy Markdown

MemberAuthor
Missing features at moment:
  • isocalendar
  • millisecond
  • microsecond
  • nanosecond
Different input / output types should also be handled.

Does this need to be timezone aware?
@jorisvandenbossche
jorisvandenbossche
commented
Apr 28, 2021

Copy link


Copy Markdown

Member
Does this need to be timezone aware?
Unfortunately yes .. But since that's a lot more complicated, and I don't think we already have tz-related functionality, for now those kernels could maybe just raise an error if the timestamp type has a tz and is thus not tz-naive?
@rok
rokforce-pushed
the
ARROW-11759
branch
4 times, most recently
from
2c4968d to
2765cefCompareMay 6, 2021 16:20
@rok
rok
commented
May 6, 2021

Copy link


Copy Markdown

MemberAuthor
It seems tests on windows are not able to download IANA timezone database or access it post-download (https://howardhinnant.github.io/date/tz.html#Installation). I'll solve it but meanwhile I I'd like to ask for review.
Open questions I have at the moment:
  • Is it possible to match kernels to zoned timestamps when registering or do we have to keep matching at runtime like so.
  • What would be a good array type for isodate?
@rok
rok
marked this pull request as ready for review
May 6, 2021 17:22
@rok
rok
commented
May 8, 2021

Copy link


Copy Markdown

MemberAuthor
I've adopted some code from iso_week.h. Would it make more sense to add iso_week.h as a vendored library instead? Should we add a note to our license? (iso_week.h is MIT licensed).
pitrou
pitrou
requested changes
May 10, 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.

Thanks for the PR @rok. There's a bunch of things that look unexpected to me, see below.
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_test.cc
Outdated

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

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

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

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

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

@jorisvandenbossche
jorisvandenbossche
commented
May 10, 2021

Copy link


Copy Markdown

Member
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
@rok
rok
commented
May 12, 2021

Copy link


Copy Markdown

MemberAuthor
General comment: the PR is quite big, and the timezone-related logic further added complexity. So from a workflow perspective (to ensure reviewing / getting this merged is manageable), could it make sense to keep the timezone-related logic for a separate PR? (didn't check the code to see whether that's actually feasible)
Keeping tz-aware logic would be fairly easy to move to a separate PR. I'll move it out.
@rok
rokforce-pushed
the
ARROW-11759
branch
8 times, most recently
from
f8c6665 to
23f51e7CompareMay 17, 2021 02:44
jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

jorisvandenbossche
jorisvandenbossche
reviewed
May 17, 2021
View reviewed changes
Comment threadcpp/src/arrow/compute/api_scalar.h
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
3 times, most recently
from
7bcc910 to
69c5760CompareMay 20, 2021 18:26
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
@jorisvandenbosschejorisvandenbossche
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.

Thanks for the updates! Did a final pass through the docs.
Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
Outdated

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

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

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

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal.cc
 {"values"}};

const FunctionDoc millisecond_doc{
 "Extract millisecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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 here the clarification about it being the number of milliseconds since the last full second, as you did for the C++ functions? (and same for microsecond/nanosecond below)
@rokrokJun 9, 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.cc
 {"values"}};

const FunctionDoc subsecond_doc{
 "Extract subsecond values",
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

Here also best to add the clarification of what subsecond is from the Subsecond docstring
@rokrokJun 9, 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.cc
 }
 return {static_cast<int64_t>(static_cast<int32_t>(y)),
 static_cast<int64_t>(trunc<weeks>(t - start).count() + 1),
 static_cast<int64_t>(weekday(ymd).iso_encoding())};
@jorisvandenbosschejorisvandenbosscheJun 9, 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.

In the DayOfWeek implementation you subtract 1 (to have 0-6), while here not. I assume that's the ISO definition? If so, we should clearly document that difference in the ISOCalendar documentation (since now it seems to imply that the "day_of_week" field is the same as the "day_of_week" kernel. Maybe also prepend it with "iso_" ?
@rokrokJun 9, 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.

Added to docs and prepended.
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok
rokforce-pushed
the
ARROW-11759
branch
2 times, most recently
from
4dc50cb to
ee53452CompareJune 9, 2021 12:03
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks for the comments and suggestions @jorisvandenbossche! :)
pitrou
pitrou
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Apply suggestions from code review
6625c14
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pitrou
pitrou
commented
Jun 9, 2021

Copy link


Copy Markdown

Member
Thank you for this PR @rok ! I believe this is ready for merging. I'll just wait for CI.
jorisvandenbossche
jorisvandenbossche
reviewed
Jun 9, 2021
View reviewed changes
Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@jorisvandenbossche
Update docs/source/cpp/compute.rst
16ef106
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Just noticed I missed this:
@jorisvandenbossche

Also, it would maybe be good to add a test for a timestamp that doesn't fit into the nanosecond range?
Do you mind if I move it to ARROW-12980?
@rok
rok
commented
Jun 9, 2021

Copy link


Copy Markdown

MemberAuthor
Actually @jorisvandenbossche what do you mean by "doesn't fit into the nanosecond range"?

Like ` "1970-01-01T00:00:59.1234567890123"?
@pitroupitrou
closed this
in
00cc41fJun 9, 2021
@rok
rok
deleted the

ARROW-11759

branch
June 9, 2021 19:04
@jorisvandenbossche
jorisvandenbossche
commented
Jun 10, 2021

Copy link


Copy Markdown

Member
Cool, this is a nice start for datetime kernels!
@thisisnic opened a JIRA for expsosing those in R as well (https://issues.apache.org/jira/browse/ARROW-13022)
what do you mean by "doesn't fit into the nanosecond range"?
I meant a date that falls outside the date range of 1677-09-21 - 2262-04-11 (the range that ns resolution can cover).
@rok
rok
commented
Jun 10, 2021

Copy link


Copy Markdown

MemberAuthor
Thanks @jorisvandenbossche@pitrou for the help and feedback! I'm very glad this is merged! :)
@rokrok
mentioned this pull request
Jun 10, 2021
 Closed

@asfimportasfimport
mentioned this pull request
Aug 4, 2021

Closed

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@pitroupitroupitrou approved these changes

Assignees


No one assigned


Labels

Component: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@rok@jorisvandenbossche@pitrou