jonkeane commented on a change in pull request #11758:
URL: https://github.com/apache/arrow/pull/11758#discussion_r754658762
##########
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:
For example: when/if we implement other decimal precisions we won't have
to gate on each of those in the same way.
##########
File path: r/R/type.R
##########
@@ -187,8 +187,11 @@ NestedType <- R6Class("NestedType", inherit = DataType)
#' @param timezone For `timestamp()`, an optional time zone string.
#' @param byte_width byte width for `FixedSizeBinary` type.
#' @param list_size list size for `FixedSizeList` type.
-#' @param precision For `decimal()`, precision
-#' @param scale For `decimal()`, scale
+#' @param precision For `decimal()`, the number of significant digits
+#' the arrow `decimal` type can represent. Currently `decimal()` is mapped
+#' to `DecimalType128`, having a maximum precision of 38 significant digits.
Review comment:
Is decimal256 exposed? Should it be? If it is / if we do, should we
mention what the max precision is for it?
##########
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:
I'm ok adding these, but I'm curious if there's something specifically
wrong with the error messages we are seeing from C++ that we're addressing with
this re-write of the error message? They seem ~equivalent to me in
readability/understandability but I might be too far down this rabbit hole.
_In general_ if the C++ error is understandable, we should defer to that so
that we don't need to have a bunch of error handling code duplicated at these
two different layers.
##########
File path: r/R/type.R
##########
@@ -187,8 +187,11 @@ NestedType <- R6Class("NestedType", inherit = DataType)
#' @param timezone For `timestamp()`, an optional time zone string.
#' @param byte_width byte width for `FixedSizeBinary` type.
#' @param list_size list size for `FixedSizeList` type.
-#' @param precision For `decimal()`, precision
-#' @param scale For `decimal()`, scale
+#' @param precision For `decimal()`, the number of significant digits
+#' the arrow `decimal` type can represent. Currently `decimal()` is mapped
+#' to `DecimalType128`, having a maximum precision of 38 significant digits.
+#' @param scale For `decimal()`, the number of digits after the decimal
+#' point. It can be negative.
Review comment:
> It can be negative.
Would it make sense to expand on why one might want to do this? The argument
text itself is probably not the right place, but adding a paragraph above in
the description might be nice.
--
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]
