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-14943: [R] Bindings for lubridate's ddays, dhours, dminutes, dmonths, dweeks, dyears #12610

Closed
wants to merge 9 commits into from

Conversation

AlenkaF
Copy link
Member

@AlenkaF AlenkaF commented Mar 11, 2022

No description provided.

@github-actions
Copy link

@AlenkaF
Copy link
Member Author

AlenkaF commented Mar 11, 2022

This PR should be updated to use as.difftime when #12506 is merged.

@AlenkaF AlenkaF marked this pull request as draft March 11, 2022 14:53
@AlenkaF
Copy link
Member Author

AlenkaF commented Apr 5, 2022

Left the bindings as were and decided not to use as.difftime after talking to Dragos about it.

@AlenkaF AlenkaF marked this pull request as ready for review April 5, 2022 12:44
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you! I just suggested a few changes to match the style of other default arguments in this file.

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
@AlenkaF
Copy link
Member Author

AlenkaF commented Apr 11, 2022

Thanks for reviewing @paleolimbot! I should have noticed I was still in Python land ... 🤦‍♀️ :)

@AlenkaF
Copy link
Member Author

AlenkaF commented Apr 11, 2022

I added the text to the News.md in #12855.

Also got quite some linter corrections when running make style (to the code not changed by me). Didn't want to add confusion so didn't commit the changes. I could run it again on master and make a minor PR for it?

@paleolimbot
Copy link
Member

All good! I'm new to how development in the arrow R package works, but I think style changes that are outside the scope of the function that is getting added/modified should be a separate PR. I've never used make style because it doesn't run on my computer...in ggplot2 we did restyling one PR at a time (rather than running a restyle on the whole package since that has a heavy git change footprint).

@amol-
Copy link
Member

amol- commented Apr 13, 2022

@AlenkaF there seem to be a few failing tests on windows, are they related to the PR?

@AlenkaF
Copy link
Member Author

AlenkaF commented Apr 13, 2022

Oh, yes, they are related. Will have a look, thanks!

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved
@dragosmg
Copy link
Contributor

Looking good. One small suggestion. Thanks for tackling the cyclomatic complexity.

Copy link
Contributor

@dragosmg dragosmg left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for all the work! @thisisnic @jonkeane would you mind having a look and merging?

@thisisnic thisisnic self-requested a review April 14, 2022 16:45
Copy link
Member

@thisisnic thisisnic left a comment

Choose a reason for hiding this comment

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

Comments resolved, tests pass, and values used match those in lubridate, so looks good to me!

@thisisnic thisisnic closed this in 69c28de Apr 14, 2022
@AlenkaF AlenkaF deleted the ARROW-14943 branch April 15, 2022 04:06
@ursabot
Copy link

ursabot commented Apr 15, 2022

Benchmark runs are scheduled for baseline = a63ee07 and contender = 69c28de. 69c28de 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 ⬇️1.97% ⬆️0.04%] test-mac-arm
[Finished ⬇️0.71% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️2.76% ⬆️0.26%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/514| 69c28de8 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/501| 69c28de8 test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/500| 69c28de8 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/511| 69c28de8 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/513| a63ee07f ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/500| a63ee07f test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/499| a63ee07f ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/510| a63ee07f 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

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

6 participants