Skip to content

ARROW-16253: [R] Helper function for casting from float to duration via int64() #13055

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.

Sign up for GitHub

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

Jump to bottom
Closed
dragosmg wants to merge 7 commits into from
Closed

ARROW-16253: [R] Helper function for casting from float to duration via int64() #13055

dragosmg wants to merge 7 commits into from

Conversation

@dragosmg
Copy link
Contributor

@dragosmg dragosmg commented May 3, 2022

No description provided.

@github-actions
Copy link

github-actions bot commented May 3, 2022

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

@dragosmg dragosmg marked this pull request as ready for review May 3, 2022 13:53
@thisisnic thisisnic requested a review from jonkeane May 5, 2022 14:24
jonkeane
jonkeane reviewed May 6, 2022
View reviewed changes
Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Looking through, I think I found a few other places this might be helpful. What about the followin?

arrow/r/R/dplyr-funcs-datetime.R

Lines 415 to 417 in 3a646b3

# complex casting only due to cast type restrictions: time64 -> int64 -> duration(us)
# and then we cast to duration ("s") at the end
x <- x$cast(time64("us"))$cast(int64())$cast(duration("us"))

arrow/r/R/dplyr-funcs-datetime.R

Lines 354 to 355 in 3a646b3

delta <- delta$cast(int64())
start + delta$cast(duration("s"))

We also mention that issue at

arrow/r/R/dplyr-datetime-helpers.R

Line 146 in 3a646b3

# https://issues.apache.org/jira/browse/ARROW-16253 (helper function for
is it possible to extend this helper to help there too? (plus any other of the places we see this same pattern?)

@dragosmg
Copy link
Contributor Author

dragosmg commented May 9, 2022

Thanks @jon. I should've probably offered some details on why I didn't use make_duration() in those places.

arrow/r/R/dplyr-funcs-datetime.R

Lines 415 to 417 in 3a646b3

# complex casting only due to cast type restrictions: time64 -> int64 -> duration(us)
# and then we cast to duration ("s") at the end
x <- x$cast(time64("us"))$cast(int64())$cast(duration("us"))

I had added a comment in b4ccc08, on why make_duration() cannot be used there. We need the casting chain for a second reason, i.e. to get the units right. Switching to make_duration() would involve adding another transformation to account for the change in measurement unit. There is a relevant conversation over at ARROW-15996.

for

arrow/r/R/dplyr-funcs-datetime.R

Lines 354 to 355 in 3a646b3

delta <- delta$cast(int64())
start + delta$cast(duration("s"))

Done! That was an oversight

for

arrow/r/R/dplyr-datetime-helpers.R

Line 146 in 3a646b3

# https://issues.apache.org/jira/browse/ARROW-16253 (helper function for

we can't really use make_duration() there (as only the cast to int64(), and not the cast to duration("s") is needed) so I deleted the comment referencing ARROW-16253.

@dragosmg dragosmg requested a review from jonkeane May 9, 2022 08:10
@dragosmg
Copy link
Contributor Author

dragosmg commented May 9, 2022

I think this is ready for another look.

jonkeane
jonkeane reviewed May 9, 2022
View reviewed changes
jonkeane
jonkeane approved these changes May 9, 2022
View reviewed changes
Copy link
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

This is looking good to go (with a slight clean up of a comment).

I've not looked extensively to see if there are other places we could use this like the handful I noted, have you taken a look through the date-time codebase to see if there are more?

@dragosmg
Copy link
Contributor Author

dragosmg commented May 11, 2022

I haven't found more places in which to use this.

@thisisnic thisisnic closed this in 101f63e May 11, 2022
@ursabot
Copy link

ursabot commented May 12, 2022

Benchmark runs are scheduled for baseline = 824f58f and contender = 101f63e. 101f63e 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 ⬇️0.62% ⬆️0.04%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.71%] ursa-i9-9960x
[Finished ⬇️0.2% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 101f63ec ec2-t3-xlarge-us-east-2
[Failed] 101f63ec test-mac-arm
[Finished] 101f63ec ursa-i9-9960x
[Finished] 101f63ec ursa-thinkcentre-m75q
[Finished] 824f58f7 ec2-t3-xlarge-us-east-2
[Finished] 824f58f7 test-mac-arm
[Failed] 824f58f7 ursa-i9-9960x
[Finished] 824f58f7 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 duration_casting_helper branch May 13, 2022 08:16
@asfimport asfimport mentioned this pull request May 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonkeane jonkeane jonkeane approved these changes

Assignees

No one assigned

Labels

Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dragosmg @ursabot @jonkeane