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.

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

ARROW-13886 [R] Expand documentation for decimal() #11758

Closed
wants to merge 11 commits into from
Closed

ARROW-13886 [R] Expand documentation for decimal() #11758

wants to merge 11 commits into from

Conversation

dragosmg
Copy link
Contributor

@dragosmg dragosmg commented Nov 22, 2021 •
edited
Loading

First attempt at this. Will be followed by:

  • opening Jira tickets to:
    • deprecate decimal()
    • implement decimal128() and decimal256()
  • expand unit tests both for data types and Arrays

Tickets generated:

@dragosmg
added 2 checks for precision and improved @param entries for `precisi…
eca6b1a 
…on` and `scale` + docs + updated unit tests
@github-actions
Copy link

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

thisisnic
thisisnic reviewed Nov 22, 2021
View reviewed changes
r/R/type.R Outdated Show resolved Hide resolved
thisisnic
thisisnic reviewed Nov 22, 2021
View reviewed changes
r/R/type.R Outdated Show resolved Hide resolved
thisisnic
thisisnic reviewed Nov 22, 2021
View reviewed changes
r/R/type.R Outdated Show resolved Hide resolved
thisisnic
thisisnic reviewed Nov 22, 2021
View reviewed changes
r/R/type.R Outdated Show resolved Hide resolved
thisisnic
thisisnic reviewed Nov 22, 2021
View reviewed changes
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.

A few pedantic changes but otherwise looks good. Won't approve yet as I think you're still making changes.

jonkeane
jonkeane requested changes Nov 22, 2021
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.

Thanks for this, looking good so far, a few questions / comments

r/R/type.R Show resolved Hide resolved
r/R/type.R Outdated Show resolved Hide resolved
r/tests/testthat/test-data-type.R Outdated Show resolved Hide resolved
@dragosmg dragosmg requested a review from jonkeane November 23, 2021 14:04
jonkeane
jonkeane approved these changes Nov 23, 2021
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 good — a few small comments

r/R/type.R Show resolved Hide resolved
r/tests/testthat/test-data-type.R Outdated Show resolved Hide resolved
@dragosmg
Copy link
Contributor Author

Raised a Jira to improve the error messaging and reverted to use the error messages as surfaced from the C++ code

pitrou
pitrou reviewed Nov 24, 2021
View reviewed changes
r/R/type.R Show resolved Hide resolved
@dragosmg dragosmg requested a review from jonkeane November 24, 2021 16:00
@thisisnic thisisnic closed this in d722f50 Nov 27, 2021
@ursabot
Copy link

ursabot commented Nov 27, 2021 •
edited
Loading

Benchmark runs are scheduled for baseline = 35b3567 and contender = d722f50. d722f50 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] ursa-i9-9960x
[Finished ⬇️0.44% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

@dragosmg dragosmg deleted the ARROW-13886_decimal_docs branch December 6, 2021 16:14
@asfimport asfimport mentioned this pull request Nov 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisisnic thisisnic thisisnic left review comments

@pitrou pitrou pitrou left review comments

@jonkeane jonkeane Awaiting requested review from jonkeane

Assignees
No one assigned
Labels
Component: R
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dragosmg @ursabot @jonkeane @pitrou @thisisnic