Skip to content

ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240

Closed
dragosmg wants to merge 56 commits into
apache:masterfrom
dragosmg:timestampts_missing_timezone
Closed

ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom
dragosmg:timestampts_missing_timezone

Conversation

@dragosmg

@dragosmgdragosmg commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actionsBot commented Jan 24, 2022

Copy link
Copy Markdown

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

@github-actions

github-actionsBot commented Jan 24, 2022

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

jonkeane
jonkeane reviewed Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R Outdated
jonkeane
jonkeane reviewed Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R Outdated
dragosmg
dragosmg commented Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R Outdated
@dragosmg

dragosmg commented Jan 26, 2022

Copy link
Copy Markdown
ContributorAuthor

I think this is almost ready to go. 2 questions remain in my mind:

  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?

@dragosmg
dragosmgforce-pushed the timestampts_missing_timezone branch from da5f7f6 to 89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane requested changes Jan 26, 2022
View reviewed changes

@jonkeanejonkeane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.

I also wonder if we should actually be making this change at

arrow/r/src/type_infer.cpp

Lines 73 to 77 in 01855c7

auto tzone_sexp = Rf_getAttrib(x, symbols::tzone);
if (Rf_isNull(tzone_sexp)) {
returntimestamp(TimeUnit::MICRO);
} else {
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0)));

which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation

Comment threadr/tests/testthat/test-Array.R Outdated
jonkeane
jonkeane reviewed Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R Outdated
@jonkeane

jonkeane commented Jan 26, 2022

Copy link
Copy Markdown
Member

Any thoughts about what I suggested in #12240 (review) ?

@dragosmg

dragosmg commented Jan 26, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks

@jonkeanejonkeane changed the title ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane changed the title ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg commented Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp Outdated
jonkeane
jonkeane reviewed Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp Outdated
jonkeane
jonkeane reviewed Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp Outdated
@dragosmg

dragosmg commented Feb 1, 2022

Copy link
Copy Markdown
ContributorAuthor

It seems some unit tests are failing on Windows due to the timezone database not being found.

@jonkeane

jonkeane commented Feb 1, 2022

Copy link
Copy Markdown
Member

I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?

arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42 in 56e270f

# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL

might be relevant here

@dragosmg
dragosmgforce-pushed the timestampts_missing_timezone branch from 341640c to aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg

dragosmg commented Feb 2, 2022

Copy link
Copy Markdown
ContributorAuthor

@jonkeane could the solution be to simply skip the failing tests on Windows?

@jonkeane

jonkeane commented Feb 3, 2022

Copy link
Copy Markdown
Member

@jonkeane could the solution be to simply skip the failing tests on Windows?

We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)

jonkeane
jonkeane reviewed Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R Outdated
@jonkeanejonkeane mentioned this pull request Feb 3, 2022
Closed
@dragosmg
dragosmg marked this pull request as ready for review February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed the timestampts_missing_timezone branch from 8708968 to c558d89CompareFebruary 22, 2022 10:19
@dragosmg

dragosmg commented Feb 23, 2022

Copy link
Copy Markdown
ContributorAuthor

Currently blocked by ARROW-13168.

dragosmg added a commit to dragosmg/arrow that referenced this pull request Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg marked this pull request as ready for review March 30, 2022 14:03
@dragosmg

dragosmg commented Mar 30, 2022

Copy link
Copy Markdown
ContributorAuthor

@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.

@rok

rok commented Mar 30, 2022

Copy link
Copy Markdown
Member

Yup, here's the magic: #12536

@dragosmg

dragosmg commented Mar 30, 2022

Copy link
Copy Markdown
ContributorAuthor

Yep, I know. I was waiting for that PR to get merged. 😉

dragosmg
dragosmg commented Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R Outdated
jonkeane
jonkeane requested changes Mar 30, 2022
View reviewed changes

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

Woo, I like simple changes like this that just work in the end 😄

A few questions | comments

Comment threadr/tests/testthat/helper-skip.R Outdated
Comment threadr/tests/testthat/test-Array.R Outdated
Comment threadr/tests/testthat/test-Array.R Outdated
@dragosmg
dragosmg requested a review from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane requested changes Mar 31, 2022
View reviewed changes

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

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)

Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane reviewed Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R Outdated
@AlenkaF

AlenkaF commented Apr 5, 2022

Copy link
Copy Markdown
Member

Looks good to me 👍

@ursabot

ursabot commented Apr 11, 2022

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x
[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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

@dragosmg
dragosmg deleted the timestampts_missing_timezone branch April 12, 2022 12:44
This was referenced Apr 11, 2022
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone by dragosmg · Pull Request #12240 · apache/arrow · GitHub
Skip to content
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240
Closed
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Closed
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Conversation
@dragosmg
@dragosmgdragosmg
commented
Jan 24, 2022

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone by dragosmg · Pull Request #12240 · apache/arrow · GitHub
Skip to content
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240
Closed
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Closed
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Conversation
@dragosmg
@dragosmgdragosmg
commented
Jan 24, 2022

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone by dragosmg · Pull Request #12240 · apache/arrow · GitHub
Skip to content
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240
Closed
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Closed
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Conversation
@dragosmg
@dragosmgdragosmg
commented
Jan 24, 2022

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone by dragosmg · Pull Request #12240 · apache/arrow · GitHub
Skip to content
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240
Closed
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Closed
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Conversation
@dragosmg
@dragosmgdragosmg
commented
Jan 24, 2022

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone by dragosmg · Pull Request #12240 · apache/arrow · GitHub
Skip to content
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240
Closed
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Closed
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Conversation
@dragosmg
@dragosmgdragosmg
commented
Jan 24, 2022

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone by dragosmg · Pull Request #12240 · apache/arrow · GitHub
Skip to content
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone - #12240
Closed
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Closed
ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone#12240
dragosmg wants to merge 56 commits into
apache:masterfrom 
dragosmg:timestampts_missing_timezone
Conversation
@dragosmg
@dragosmgdragosmg
commented
Jan 24, 2022

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche

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

Copy link


Copy Markdown

Contributor
 No description provided. 
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-14442
@github-actions
github-actionsBot
commented
Jan 24, 2022

Copy link


Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.
jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 24, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

dragosmg
dragosmg
commented
Jan 26, 2022
View reviewed changes
Comment threadr/R/array.R
Outdated

@dragosmg
dragosmg
commented
Jan 26, 2022

Copy link


Copy Markdown

ContributorAuthor
I think this is almost ready to go. 2 questions remain in my mind:
  1. do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
  2. do we want to inform the user about the change in timezone - from timezoneless to system time zone?
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
da5f7f6 to
89834d6CompareJanuary 26, 2022 18:05
jonkeane
jonkeane
requested changes
Jan 26, 2022
View reviewed changes
@jonkeanejonkeane
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Thanks for all the digging on this. I have a few comments about adding a few more tests.
I also wonder if we should actually be making this change at
arrow/r/src/type_infer.cpp

Lines 73 to 77
in
01855c7
auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
if (Rf_isNull(tzone_sexp)) { 
returntimestamp(TimeUnit::MICRO); 
 } else { 
returntimestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); 
which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation
Comment threadr/tests/testthat/test-Array.R
Outdated

jonkeane
jonkeane
reviewed
Jan 26, 2022
View reviewed changes
Comment threadr/tests/testthat/test-Array.R
Outdated

@jonkeane
jonkeane
commented
Jan 26, 2022

Copy link


Copy Markdown

Member
Any thoughts about what I suggested in #12240 (review) ?
@dragosmg
dragosmg
commented
Jan 26, 2022•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks
@jonkeanejonkeane
changed the title
ARROW-14442 [R]behaviour when converting timestamps with "" as tzoneARROW-14442: [R] behaviour when converting timestamps with "" as tzoneJan 28, 2022
@jonkeanejonkeane
changed the title
ARROW-14442: [R] behaviour when converting timestamps with "" as tzoneARROW-14442: [R] fix behaviour when converting timestamps with "" as tzoneJan 28, 2022
dragosmg
dragosmg
commented
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

jonkeane
jonkeane
reviewed
Feb 1, 2022
View reviewed changes
Comment threadr/src/type_infer.cpp
Outdated

@dragosmg
dragosmg
commented
Feb 1, 2022

Copy link


Copy Markdown

ContributorAuthor
It seems some unit tests are failing on Windows due to the timezone database not being found.
@jonkeane
jonkeane
commented
Feb 1, 2022

Copy link


Copy Markdown

Member
I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?
arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42
in
56e270f
# test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent
# so we can run some tests on Windows, but this skirts around
# https://issues.apache.org/jira/browse/ARROW-13588
# That issue is tough because in C++, "" is the "no timezone" value
# due to static typing, so we can't distinguish a literal "" from NULL
might be relevant here
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
341640c to
aa9f33eCompareFebruary 2, 2022 13:50
@dragosmg
dragosmg
commented
Feb 2, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane could the solution be to simply skip the failing tests on Windows?
@jonkeane
jonkeane
commented
Feb 3, 2022

Copy link


Copy Markdown

Member
@jonkeane could the solution be to simply skip the failing tests on Windows?
We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)
jonkeane
jonkeane
reviewed
Feb 4, 2022
View reviewed changes
Comment threadr/tests/testthat/test-dplyr-funcs-datetime.R
Outdated

@jonkeanejonkeane
mentioned this pull request
Feb 3, 2022
 Closed

@dragosmg
dragosmg
marked this pull request as ready for review
February 22, 2022 09:41
@dragosmg
dragosmgforce-pushed
the
timestampts_missing_timezone
branch
from
8708968 to
c558d89CompareFebruary 22, 2022 10:19
@dragosmg
dragosmg
commented
Feb 23, 2022

Copy link


Copy Markdown

ContributorAuthor
Currently blocked by ARROW-13168.
dragosmg added a commit
to dragosmg/arrow
that referenced
this pull request
Mar 4, 2022
@dragosmg
added TODO to revisit the casting step onceapache#12240is merged
03eb111
@dragosmg
dragosmg
marked this pull request as ready for review
March 30, 2022 14:03
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.
@rok
rok
commented
Mar 30, 2022

Copy link


Copy Markdown

Member
Yup, here's the magic: #12536
@dragosmg
dragosmg
commented
Mar 30, 2022

Copy link


Copy Markdown

ContributorAuthor
Yep, I know. I was waiting for that PR to get merged. 😉
dragosmg
dragosmg
commented
Mar 30, 2022
View reviewed changes
Comment threadr/tests/testthat/helper-skip.R
Outdated

jonkeane
jonkeane
requested changes
Mar 30, 2022
View reviewed changes
@jonkeanejonkeane
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.

Woo, I like simple changes like this that just work in the end 😄
A few questions | comments
Comment threadr/tests/testthat/helper-skip.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

Comment threadr/tests/testthat/test-Array.R
Outdated

@dragosmg
dragosmg
requested a review
from jonkeaneMarch 31, 2022 09:08
jonkeane
jonkeane
requested changes
Mar 31, 2022
View reviewed changes
@jonkeanejonkeane
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.

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
Comment threadr/tests/testthat/test-Array.R
jonkeane
jonkeane
reviewed
Apr 1, 2022
View reviewed changes
Comment threadr/R/arrow-package.R
Outdated

@AlenkaF
AlenkaF
commented
Apr 5, 2022

Copy link


Copy Markdown

Member
Looks good to me 👍
@ursabot
ursabot
commented
Apr 11, 2022

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c 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 ⬇️1.0% ⬆️0.25%] test-mac-arm

[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x

[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484|633687c1 ec2-t3-xlarge-us-east-2>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469|633687c1 test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470|633687c1 ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479|633687c1 ursa-thinkcentre-m75q>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483|e453ffef ec2-t3-xlarge-us-east-2>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468|e453ffef test-mac-arm>

[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469|e453ffef ursa-i9-9960x>

[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478|e453ffef 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
@dragosmg
dragosmg
deleted the

timestampts_missing_timezone

branch
April 12, 2022 12:44

This was referenced Apr 11, 2022

Closed


Closed

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

Reviewers

@rokrokrok left review comments
@jonkeanejonkeanejonkeane requested changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche left review comments

Assignees


No one assigned


Labels

Component: R

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@dragosmg@jonkeane@rok@AlenkaF@ursabot@jorisvandenbossche