Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-17386: [R] strptime tests not robust across platforms #13854

Merged
merged 4 commits into from
Sep 13, 2022

Conversation

rok
Copy link
Member

@rok rok commented Aug 11, 2022

This is to resolve ARROW-17386.

@rok rok changed the title [R] strptime tests not robust across platforms ARROW-17386: [R] strptime tests not robust across platforms Aug 11, 2022
@github-actions
Copy link

Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

The other thing I'd like to see is assertions that the result of strptime is not all NA. My guess about the CRAN issue is that we're testing a token that isn't supported anywhere else (in both R and Arrow), and the difference on his machine is that it does parse successfully in R bust still not in Arrow.

r/tests/testthat/test-dplyr-funcs-datetime.R Show resolved Hide resolved
r/tests/testthat/test-dplyr-funcs-datetime.R Show resolved Hide resolved
@kou
Copy link
Member

kou commented Aug 12, 2022

You may want to export arrow::internal::kStrptimeSupportsZone as a public API and use it: https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/value_parsing.h#L774

@rok
Copy link
Member Author

rok commented Aug 12, 2022

You may want to export arrow::internal::kStrptimeSupportsZone as a public API and use it: https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/value_parsing.h#L774

That's a good point! However lubridate's strptime supports %Z flag for strftime only (see docs), so I don't think we need kStrptimeSupportsZone for this change. I opened ARROW-17398 to not lose the idea.

@rok
Copy link
Member Author

rok commented Sep 9, 2022

Thanks for the review @nealrichardson ! I've addressed your comments and I think this is ready for another review round.

@rok
Copy link
Member Author

rok commented Sep 12, 2022

CI issues don't seem related.

@rok rok merged commit 4ae26d1 into apache:master Sep 13, 2022
@rok rok deleted the ARROW-17386 branch September 13, 2022 00:11
@ursabot
Copy link

ursabot commented Sep 13, 2022

Benchmark runs are scheduled for baseline = 6c675c3 and contender = 4ae26d1. 4ae26d1 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
[Finished ⬇️0.41% ⬆️0.07%] test-mac-arm
[Failed ⬇️0.56% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.07% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 4ae26d1b ec2-t3-xlarge-us-east-2
[Finished] 4ae26d1b test-mac-arm
[Failed] 4ae26d1b ursa-i9-9960x
[Finished] 4ae26d1b ursa-thinkcentre-m75q
[Finished] 6c675c35 ec2-t3-xlarge-us-east-2
[Finished] 6c675c35 test-mac-arm
[Failed] 6c675c35 ursa-i9-9960x
[Finished] 6c675c35 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

@ursabot
Copy link

ursabot commented Sep 13, 2022

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

@rok
Copy link
Member Author

rok commented Sep 13, 2022

@jonkeane We only change the test here. Is the regression misattributed or are these tests used in tcph or <insert third option>?

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…3854)

This is to resolve [ARROW-17386](https://issues.apache.org/jira/browse/ARROW-17386).

Lead-authored-by: Rok <rok@mihevc.org>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok <rok@mihevc.org>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
…3854)

This is to resolve [ARROW-17386](https://issues.apache.org/jira/browse/ARROW-17386).

Lead-authored-by: Rok <rok@mihevc.org>
Co-authored-by: Rok Mihevc <rok@mihevc.org>
Signed-off-by: Rok <rok@mihevc.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants