Skip to content

ARROW-16255: [R] Reorganise the datetime bindings - #13029

Closed
dragosmg wants to merge 10 commits into
apache:masterfrom
dragosmg:datetime_bindings_reorg
Closed

ARROW-16255: [R] Reorganise the datetime bindings#13029
dragosmg wants to merge 10 commits into
apache:masterfrom
dragosmg:datetime_bindings_reorg

Conversation

@dragosmg

@dragosmgdragosmg commented Apr 29, 2022

Copy link
Copy Markdown
Contributor

The purpose of this PR is to reorganise the datetime bindings.

Why?

  • some are in files where one wouldn't think to look (e.g. in R/dplyr-funcs-type.R)
  • the are a bunch of somewhat scattered helper functions
  • some of the register_bindings_...() functions are too complex and trigger the cyclocomp lint

What?

  • create a separate file for the datetime helpers, called R/dplyr-datetime-helpers.R
  • all bindings are in dplyr-funcs-datetime.R (with the exception of leap_year, which was moved to expressions.R)
  • all tests are in test-dplyr-funcs-datetime.R

Results

  • cyclomatic complexity for the dplyr-funcs-datetime.R reduced to 21 (from 26)
More details

BindingOld registering functionNew registering function
strptimeregister_bindings_datetime()register_bindings_datetime_utility()
strftimeregister_bindings_datetime()register_bindings_datetime_utility()
format_ISO8601register_bindings_datetime()register_bindings_datetime_utility()
secondregister_bindings_datetime()register_bindings_datetime_components()
wdayregister_bindings_datetime()register_bindings_datetime_components()
weekregister_bindings_datetime()register_bindings_datetime_components()
monthregister_bindings_datetime()register_bindings_datetime_components()
is.Dateregister_bindings_datetime()register_bindings_datetime_utility()
is.instantregister_bindings_datetime()register_bindings_datetime_utility()
is.timepointregister_bindings_datetime()register_bindings_datetime_utility()
is.POSIXctregister_bindings_datetime()register_bindings_datetime_utility()
leap_yearregister_bindings_datetime()
amregister_bindings_datetime()register_bindings_datetime_components()
pmregister_bindings_datetime()register_bindings_datetime_components()
tzregister_bindings_datetime()register_bindings_datetime_components()
semesterregister_bindings_datetime()register_bindings_datetime_components()
dateregister_bindings_datetime()register_bindings_datetime_utility()
make_datetimeregister_bindings_duration()register_bindings_datetime_conversion()
make_dateregister_bindings_duration()register_bindings_datetime_conversion()
ISOdatetimeregister_bindings_duration()register_bindings_datetime_conversion()
ISOdateregister_bindings_duration()register_bindings_datetime_conversion()
difftimeregister_bindings_duration()register_bindings_duration()
as.difftimeregister_bindings_duration()register_bindings_duration()
decimal_dateregister_bindings_duration()register_bindings_datetime_conversion()
date_decimalregister_bindings_duration()register_bindings_datetime_conversion()
duration_helpers_map_factoryregister_bindings_duration_helpers()register_bindings_duration_helpers()
dpicosecondsregister_bindings_duration_helpers()register_bindings_duration_helpers()
make_difftime register_bindings_difftime_constructors()register_bindings_duration_constructor()
as.Dateregister_bindings_type_cast()register_bindings_datetime_conversion()
as_date register_bindings_type_cast()register_bindings_datetime_conversion()
as_datetime register_bindings_type_cast()register_bindings_datetime_conversion()

@github-actions

Copy link
Copy Markdown

@dragosmg
dragosmg marked this pull request as ready for review April 29, 2022 13:06

@thisisnicthisisnic 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 - thanks very much for re-organising these, will make it a lot easier for newer contributors to find their way around the project.

@ursabot

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7f6b074 and contender = 893faa7. 893faa7 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.43% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.32% ⬆️0.08%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 893faa74 ec2-t3-xlarge-us-east-2
[Finished] 893faa74 test-mac-arm
[Finished] 893faa74 ursa-i9-9960x
[Finished] 893faa74 ursa-thinkcentre-m75q
[Finished] 7f6b074b ec2-t3-xlarge-us-east-2
[Finished] 7f6b074b test-mac-arm
[Finished] 7f6b074b ursa-i9-9960x
[Finished] 7f6b074b 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
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dragosmg@ursabot@thisisnic