-
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
Changes from all commits
055e964
bba7838
a3b7fe1
b7b350b
23d2ba8
1249367
6174645
79f1886
b75c573
e1e657b
f6b31b4
6df375d
1a6dff6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -474,16 +474,15 @@ test_that("DictionaryType validation", { | |
| }) | ||
|
|
||
| test_that("decimal type and validation", { | ||
| expect_r6_class(decimal(4, 2), "Decimal128Type") | ||
| expect_r6_class(decimal(4, 2), "Decimal32Type") | ||
| expect_r6_class(decimal(14, 2), "Decimal64Type") | ||
| expect_r6_class(decimal(22, 2), "Decimal128Type") | ||
| expect_r6_class(decimal(39, 2), "Decimal256Type") | ||
|
|
||
| expect_error(decimal("four"), "`precision` must be an integer") | ||
| expect_error(decimal(4, "two"), "`scale` must be an integer") | ||
| expect_error(decimal(NA, 2), "`precision` must be an integer") | ||
| expect_error(decimal(4, NA), "`scale` must be an integer") | ||
| # TODO remove precision range tests below once functionality is tested in C++ (ARROW-15162) | ||
| expect_error(decimal(0, 2), "Invalid: Decimal precision out of range [1, 38]: 0", fixed = TRUE) | ||
| expect_error(decimal(100, 2), "Invalid: Decimal precision out of range [1, 76]: 100", fixed = TRUE) | ||
|
|
||
| # decimal() creates either decimal128 or decimal256 based on precision | ||
| expect_identical(class(decimal(38, 2)), class(decimal128(38, 2))) | ||
|
|
@@ -497,10 +496,6 @@ test_that("decimal type and validation", { | |
| expect_error(decimal128(4, NA), "`scale` must be an integer") | ||
| expect_error(decimal128(3:4, NA), "`precision` must have size 1. not size 2") | ||
| expect_error(decimal128(4, 2:3), "`scale` must have size 1. not size 2") | ||
| # TODO remove precision range tests below once functionality is tested in C++ (ARROW-15162) | ||
| expect_error(decimal128(0, 2), "Invalid: Decimal precision out of range [1, 38]: 0", fixed = TRUE) | ||
| expect_error(decimal128(100, 2), "Invalid: Decimal precision out of range [1, 38]: 100", fixed = TRUE) | ||
|
|
||
|
||
|
|
||
| expect_r6_class(decimal256(4, 2), "Decimal256Type") | ||
|
|
||
|
|
@@ -510,9 +505,6 @@ test_that("decimal type and validation", { | |
| expect_error(decimal256(4, NA), "`scale` must be an integer") | ||
| expect_error(decimal256(3:4, NA), "`precision` must have size 1. not size 2") | ||
| expect_error(decimal256(4, 2:3), "`scale` must have size 1. not size 2") | ||
| # TODO remove precision range tests below once functionality is tested in C++ (ARROW-15162) | ||
| expect_error(decimal256(0, 2), "Invalid: Decimal precision out of range [1, 76]: 0", fixed = TRUE) | ||
| expect_error(decimal256(100, 2), "Invalid: Decimal precision out of range [1, 76]: 100", fixed = TRUE) | ||
| }) | ||
|
|
||
| test_that("Binary", { | ||
|
|
||
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
Uh oh!
There was an error while loading. Please reload this page.
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)