jonkeane commented on a change in pull request #11758:
URL: https://github.com/apache/arrow/pull/11758#discussion_r755401953
##########
File path: r/R/type.R
##########
@@ -180,15 +180,30 @@ NestedType <- R6Class("NestedType", inherit = DataType)
#' types, this conversion can be disabled (so that `int64` always yields a
#' `bit64::integer64` object) by setting `options(arrow.int64_downcast =
#' FALSE)`.
+#' `decimal()` creates a `decimal128` type. Arrow decimals are fixed-point
+#' decimal numbers encoded as a scalar integer. The `precision` is the number
of
+#' significant digits that the decimal type can represent; the `scale` is the
+#' number of digits after the decimal point. For example, the number 1234.567
+#' has a precision of 7 and a scale of 3. Note that `scale` can be negative.
+#'
+#' As an example, `decimal(7, 3)` can exactly represent the numbers 1234.567
and
+#' -1234.567 (encoded internally as the 128-bit integers 1234567 and -1234567,
+#' respectively), but neither 12345.67 nor 123.4567.
+#'
+#' `decimal(5, -3)` can exactly represent the number 12345000 (encoded
+#' internally as the 128-bit integer 12345), but neither 123450000 nor 1234500.
Review comment:
This is really great, thanks! One small suggestion you can take or
leave: I think we only need to mention once that the internal encoding is an
integer. I like that we include it, but I think it might call more attention
than we need having it repeated.
I also really liked the power of 10 analogy / metaphor that you used
earlier. It would be nice to include that here (I think it's a concept that
arrow users will be familiar with and can grok easily)
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -390,8 +390,8 @@ test_that("decimal type and validation", {
expect_error(decimal(4))
expect_error(decimal(4, "two"), '"scale" must be an integer')
expect_error(decimal(NA, 2), '"precision" must be an integer')
- expect_error(decimal(0, 2), "Invalid: Decimal precision out of range: 0")
- expect_error(decimal(100, 2), "Invalid: Decimal precision out of range: 100")
+ expect_error(decimal(0, 2), "\"precision\" must be greater than 0")
+ expect_error(decimal(100, 2), "\"precision\" must be less than or equal to
38")
Review comment:
Yeah, we should raise a Jira to suggest a better warning. We don't need
to hold this PR to wait for that to happen, but maybe a comment with the Jira #
so we know to clean it up if/when the jira happens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
