-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-46719: [R] Add 32 and 64 bit Decimal types #46720
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
Conversation
r/vignettes/data_types.Rmd
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split the text up in this vignette to make it easier to edit and track edits in future, but the only actual changes I made were to this line, where I added the new decimal types in.
447c3d4 to
fc5ab6b
Compare
r/R/enums.R
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we just missing these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, went and looked the decimal ones up in the C++ and found a few more missing too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised there weren't / aren't tests that caught this? But maybe I don't totally grok what's up with these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know what these enums do, haven't traced it through, but my guess would be that they don't matter as we haven't implemented them in R? Hmm, in which case, maybe I remove? (Can do in a follow-up)
r/tests/testthat/test-data-type.R
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this cleanup, though am I misunderstanding that #30667 is still open? Or maybe this was actually implemented somewhere and we should close that issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, I confused the JIRA and GitHub numbers. Will double check the C++ code to double check though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were tests added here so we can get rid of our own tests for this, but the decimal() C++ function was deprecated in favour of smallest_decimal() so I'll need to swap that out too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we don't actually use the C++ decimal() function anyway, so no changes needed. We could look to use smallest_decimal() instead of working out which decimal it should be ourselves in the R code, but I don't think the time it would take to make this change would be worth the effort, though can open a new ticket if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would mean being able to delete some code in the R package, which is nice — let's make an issue but agreed we don't need to tackle it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - #46825
jonkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. Generally looks good, though have a few comments / questions
fc5ab6b to
1a6dff6
Compare
|
Cheers for the review @jonkeane. I've rebased to see if that fixes the linter error, but will run locally anyway to make sure. Any other changes to make before we merge? |
jonkeane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
r/R/enums.R
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised there weren't / aren't tests that caught this? But maybe I don't totally grok what's up with these
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 202787f. There were 115 benchmark results with an error:
There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
### Rationale for this change 32 and 64 bit Decimal types were added in C++ in apache#43957 but haven't been implemented in R yet ### What changes are included in this PR? Implements them in R ### Are these changes tested? Yup ### Are there any user-facing changes? Yeah, new types but also the implicit downcasting so we should think about how to communicate this if at all * GitHub Issue: apache#46719 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
Rationale for this change
32 and 64 bit Decimal types were added in C++ in #43957 but haven't been implemented in R yet
What changes are included in this PR?
Implements them in R
Are these changes tested?
Yup
Are there any user-facing changes?
Yeah, new types but also the implicit downcasting so we should think about how to communicate this if at all