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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename weekday and weekday0 kernels to to num_days_from_monday and num_days_since_sunday #2066

Merged
merged 1 commit into from Jul 14, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 14, 2022

Which issue does this PR close?

Closes #2065

Rationale for this change

Depending on the usecase sometimes one wants the days of week to start from Sunday or Monday. Dates 馃し

@ovr added the weekday0 kernel in #2052

But now the names are weekday and weekday0 which I think might be confusing to the average reader

What changes are included in this PR?

Rename the kernels to follow the chrono naming scheme num_days_from_sunday and num_days_from_monday

https://docs.rs/chrono/0.4.19/chrono/enum.Weekday.html#method.num_days_from_sunday
https://docs.rs/chrono/0.4.19/chrono/enum.Weekday.html#method.num_days_from_monday

Are there any user-facing changes?

Yes, name of the compute::kernels::temporal::weekday (which is present in arrow 18) is renamed to compute::kernels::temporal::num_days_since_monday

@alamb alamb added the api-change Changes to the arrow API label Jul 14, 2022
@alamb
Copy link
Contributor Author

alamb commented Jul 14, 2022

cc @nl5887 who contributed the original weekday kernel

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jul 14, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #2066 (43c5f97) into master (4444cb7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

鉂 Current head 43c5f97 differs from pull request most recent head 01bb3fb. Consider uploading reports for the commit 01bb3fb to get more accurate results

@@           Coverage Diff           @@
##           master    #2066   +/-   ##
=======================================
  Coverage   83.57%   83.57%           
=======================================
  Files         222      222           
  Lines       58244    58244           
=======================================
+ Hits        48675    48677    +2     
+ Misses       9569     9567    -2     
Impacted Files Coverage 螖
arrow/src/compute/kernels/temporal.rs 94.98% <100.00%> (酶)
parquet_derive/src/parquet_field.rs 66.21% <0.00%> (+0.22%) 猬嗭笍
arrow/src/datatypes/datatype.rs 65.68% <0.00%> (+0.36%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 4444cb7...01bb3fb. Read the comment docs.

@tustvold tustvold changed the title Rename weekday and weekday0 kenrels to to num_days_from_monday and days_since_sunday Rename weekday and weekday0 kernels to to num_days_from_monday and days_since_sunday Jul 14, 2022
@tustvold tustvold added the development-process Related to development process of arrow-rs label Jul 14, 2022
@tustvold tustvold merged commit 11445ee into apache:master Jul 14, 2022
18 checks passed
@tustvold
Copy link
Contributor

Labelled as development process to hide from changelog (as not released)

@ursabot
Copy link

ursabot commented Jul 14, 2022

Benchmark runs are scheduled for baseline = 4444cb7 and contender = 11445ee. 11445ee is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped 鈿狅笍 Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped 鈿狅笍 Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped 鈿狅笍 Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped 鈿狅笍 Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
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

@alamb
Copy link
Contributor Author

alamb commented Jul 14, 2022

Labelled as development process to hide from changelog (as not released)

I think since weekday is released this should be in the changelog
https://docs.rs/arrow/18.0.0/arrow/compute/kernels/temporal/fn.weekday.html

though weekday0 was not released (perhaps I can update the title)

@alamb alamb deleted the alamb/week_names branch July 14, 2022 16:22
@alamb alamb removed the development-process Related to development process of arrow-rs label Jul 14, 2022
@alamb alamb changed the title Rename weekday and weekday0 kernels to to num_days_from_monday and days_since_sunday Rename weekday and weekday0 kernels to to num_days_from_monday and num_days_since_sunday Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename weekday and weekday0 kernels to to num_days_from_monday and days_since_sunday
4 participants