GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208

Merged
westonpace merged 4 commits into
apache:mainfrom
rok:33143-get-walltime
Feb 17, 2023
Merged

GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom
rok:33143-get-walltime

Conversation

@rok

@rokrok commented Feb 15, 2023•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.

This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431

What changes are included in this PR?

This adds a local_time C++ kernel.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, local_time C++ kernel is added.

@github-actions

github-actionsBot commented Feb 15, 2023

Copy link
Copy Markdown

@rok
rokforce-pushed the 33143-get-walltime branch 2 times, most recently from cdcb8c6 to 4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed the 33143-get-walltime branch from 4a3bded to a5185a2CompareFebruary 16, 2023 16:39
@rok
rok marked this pull request as ready for review February 16, 2023 17:11
@rok
rok requested a review from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace approved these changes Feb 17, 2023
View reviewed changes

@westonpacewestonpace 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, as always, for your diligence here.

Comment threaddocs/source/cpp/compute.rst Outdated
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |

@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.

@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.

Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized

@westonpacewestonpaceFeb 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.

@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D

@rok

rok commented Feb 17, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit preview-docs

@github-actions

github-actionsBot commented Feb 17, 2023

Copy link
Copy Markdown

Revision: 115347e

Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef

TaskStatus
preview-docsGithub Actions

@rok

rok commented Feb 17, 2023

Copy link
Copy Markdown
MemberAuthor

Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling

westonpace
westonpace approved these changes Feb 17, 2023
View reviewed changes

@westonpacewestonpace 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, the description is clear to me. Two small grammar nits.

Comment threaddocs/source/cpp/compute.rst Outdated
Comment threaddocs/source/cpp/compute.rst Outdated
@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok

rok commented Feb 17, 2023

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit preview-docs

@rok

rok commented Feb 17, 2023

Copy link
Copy Markdown
MemberAuthor

Thanks @westonpace, merged the grammar changes.

@github-actions

github-actionsBot commented Feb 17, 2023

Copy link
Copy Markdown

Revision: cf2eb81

Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893

TaskStatus
preview-docsGithub Actions

@westonpace

westonpace commented Feb 17, 2023

Copy link
Copy Markdown
Member

The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).

@westonpace
westonpace merged commit daa5e26 into apache:mainFeb 17, 2023
@ursabot

ursabot commented Feb 18, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] daa5e267 ec2-t3-xlarge-us-east-2
[Failed] daa5e267 test-mac-arm
[Finished] daa5e267 ursa-i9-9960x
[Finished] daa5e267 ursa-thinkcentre-m75q
[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2
[Finished] ae1f98f5 test-mac-arm
[Finished] ae1f98f5 ursa-i9-9960x
[Finished] ae1f98f5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@jorisvandenbossche

jorisvandenbossche commented Feb 20, 2023

Copy link
Copy Markdown
Member

Nice to see this functionality!

One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?
(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).

For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.

jorisvandenbossche
jorisvandenbossche reviewed Feb 20, 2023
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.

And a few doc comments/questions

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
"cannot be found in the timezone database."),
"If timezone is not given then timezone naive timestamp in UTC are\n"
"returned. An error is returned if the values have a defined timezone\n"
"but it cannot be found in the timezone database."),

@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?
(and if intentional, why only for round and not ceil/floor?)

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
"Convert timestamp to a timezone-naive local time timestamp",
("LocalTime converts a timestamp to a local time of timestamps timezone\n"

@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?

Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
Comment on lines +1813 to +1814
"and removes timezone metadata. If input is in UTC or doesn't have\n"
"timezone metadata, it is returned as is.\n"

@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?

@rokrok mentioned this pull request Feb 20, 2023
Merged
@rok

rok commented Feb 20, 2023

Copy link
Copy Markdown
MemberAuthor

Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.

rok added a commit that referenced this pull request Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok mentioned this pull request Dec 18, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees

No one assigned

Labels

Component: C++Component: DocumentationComponent: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time

4 participants

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche

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

Skip to content
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time - #34208
Merged
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Feb 17, 2023
Merged
GH-33143: [C++] Kernel to convert timestamp with timezone to wall time#34208
westonpace merged 4 commits into
apache:mainfrom 
rok:33143-get-walltime
Conversation
@rok
@rokrok
commented
Feb 15, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have assume_timezone to go from wall time timestamp to timestamp with a timezone. We might want a reverse operation to go from timestamp with a timezone to wall time.
This is not needed for computation within Arrow, but would be needed if an application outsides of Arrow consumes wall time. E.g.: https://stackoverflow.com/questions/73275465/how-to-keep-original-datatime-in-pyarrow-table/73276431
What changes are included in this PR?
This adds a local_time C++ kernel.
Are these changes tested?
Yes.
Are there any user-facing changes?
Yes, local_time C++ kernel is added.
@github-actions
github-actionsBot
commented
Feb 15, 2023

Copy link


Copy Markdown

@rok
rokforce-pushed
the
33143-get-walltime
branch
2 times, most recently
from
cdcb8c6 to
4a3bdedCompareFebruary 16, 2023 12:30
@rok
rokforce-pushed
the
33143-get-walltime
branch
from
4a3bded to
a5185a2CompareFebruary 16, 2023 16:39
@rok
rok
marked this pull request as ready for review
February 16, 2023 17:11
@rok
rok
requested a review
from lidavidmFebruary 16, 2023 17:12
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, as always, for your diligence here.
Comment threaddocs/source/cpp/compute.rst
Outdated

+--------------------+------------+-------------------+---------------+----------------------------+-------+
| is_leap_year | Unary | Timestamp, Date | Boolean | | |
+--------------------+------------+-------------------+---------------+----------------------------+-------+
| local_time | Unary | Timestamp | Timestamp | | |
@westonpacewestonpaceFeb 17, 2023

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 we add a note? I don't think it will be obvious what this function does.
@rokrokFeb 17, 2023

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.

Good point. Added some text and moved under the Timezone handling chapter. I'll trigger a docs preview to make sure I've got the markup right.
Comment threadcpp/src/arrow/compute/api_scalar.h
/// \return the resulting datum
///
/// \since 12.0.0
/// \note API not yet finalized
@westonpacewestonpaceFeb 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I was going to suggest removing this but now I see this is everywhere. Hmm.... :( I'm not sure how true this is these days.
@rokrokFeb 17, 2023

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.

Well, you could see it as an opportunity to sync the API with substrait :D
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: 115347e
Submitted crossbow builds: ursacomputing/crossbow @ actions-ffd22963ef
TaskStatus
preview-docsGithub Actions
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Docs preview: http://crossbow.voltrondata.com/pr_docs/34208/cpp/compute.html#timezone-handling
westonpace
westonpace
approved these changes
Feb 17, 2023
View reviewed changes
@westonpacewestonpace
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, the description is clear to me. Two small grammar nits.
Comment threaddocs/source/cpp/compute.rst
Outdated

Comment threaddocs/source/cpp/compute.rst
Outdated

@rok@westonpace
Apply suggestions from code review
cf2eb81
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
@github-actions crossbow submit preview-docs
@rok
rok
commented
Feb 17, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks @westonpace, merged the grammar changes.
@github-actions
github-actionsBot
commented
Feb 17, 2023

Copy link


Copy Markdown

Revision: cf2eb81
Submitted crossbow builds: ursacomputing/crossbow @ actions-760f71e893
TaskStatus
preview-docsGithub Actions
@westonpace
westonpace
commented
Feb 17, 2023

Copy link


Copy Markdown

Member
The test failures seem unrelated (I've opened a new issue for one which appears to be an intermittent segfault).
@westonpace
westonpace
merged commit daa5e26
into
apache:mainFeb 17, 2023
@ursabot
ursabot
commented
Feb 18, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = ae1f98f and contender = daa5e26. daa5e26 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Failed ⬇️0.31% ⬆️0.03%] test-mac-arm

[Finished ⬇️0.77% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️0.54% ⬆️0.03%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] daa5e267 ec2-t3-xlarge-us-east-2

[Failed] daa5e267 test-mac-arm

[Finished] daa5e267 ursa-i9-9960x

[Finished] daa5e267 ursa-thinkcentre-m75q

[Finished] ae1f98f5 ec2-t3-xlarge-us-east-2

[Finished] ae1f98f5 test-mac-arm

[Finished] ae1f98f5 ursa-i9-9960x

[Finished] ae1f98f5 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@jorisvandenbossche
jorisvandenbossche
commented
Feb 20, 2023

Copy link


Copy Markdown

Member
Nice to see this functionality!
One comment on the name of the function: given that "local_time" could also be interpreted as returning just the time (a time32/64() type) and not both date and time (and theoretically, this could also be a kernel we would want to add at some point), we could maybe use "local_timestamp" instead?

(I know that some will day that "local timestamp" is a contradictio in terminis, and that it should be "local datetime" instead, but since we don't use "datetime" for any other kernel name and "timestamp" is the name of the type anyway, that seems the best option).
For reference, in Joda they call this toLocalDateTime (and they also have a toLocalDate and toLocalTime), in pandas the naming is confusing (tz_localize(None)), and in R's lubridate there is a local_time that does somewhat related, except that it returns the difference in seconds instead of new timestamp object.
jorisvandenbossche
jorisvandenbossche
reviewed
Feb 20, 2023
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.

And a few doc comments/questions
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc
 "cannot be found in the timezone database."),
 "If timezone is not given then timezone naive timestamp in UTC are\n"
 "returned. An error is returned if the values have a defined timezone\n"
 "but it cannot be found in the timezone database."),
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Was this change intentional? Or was it meant to be in local_time_doc?

(and if intentional, why only for round and not ceil/floor?)
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

const FunctionDoc local_time_doc{
 "Convert timestamp to a timezone-naive local time timestamp",
 ("LocalTime converts a timestamp to a local time of timestamps timezone\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Something is missing in this sentence?
Comment threadcpp/src/arrow/compute/kernels/scalar_temporal_unary.cc

Comment on lines

+1813
 to 
+1814

 "and removes timezone metadata. If input is in UTC or doesn't have\n"
 "timezone metadata, it is returned as is.\n"
@jorisvandenbosschejorisvandenbosscheFeb 20, 2023

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.

Returned "as is" is not entirely correct for UTC, as I assume the "UTC" timezone is still removed?
@rokrok
mentioned this pull request
Feb 20, 2023
 Merged

@rok
rok
commented
Feb 20, 2023

Copy link


Copy Markdown

MemberAuthor
Thanks for the review @jorisvandenbossche. I've opened a PR to address it: #34263.
rok added a commit
that referenced
this pull request
Feb 23, 2023
@rok
GH-33143: [C++] Naming and doc/test changes for local_time compute ke…
863cdd4
…rnel (#34263)
### Rationale for this change
A better naming for local_time kernel was proposed in post merge review of #34208.
### What changes are included in this PR?
Change `local_time` to `local_timestamp` and related docs/test changes.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Changing `local_time` to `local_timestamp` is a user facing change. But since it was not yet released we can probably treat it as non-breaking.
* Closes: #33143
Authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok Mihevc <rok@mihevc.org>
@rokrok
mentioned this pull request
Dec 18, 2023
 Merged

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

Reviewers

@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments
@westonpacewestonpacewestonpace approved these changes
@AlenkaFAlenkaFAwaiting requested review from AlenkaFAlenkaF is a code owner
@lidavidmlidavidmAwaiting requested review from lidavidm

Assignees


No one assigned


Labels

Component: C++Component: DocumentationComponent: Python

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Kernel to convert timestamp with timezone to wall time


4 participants

@rok@westonpace@ursabot@jorisvandenbossche