pitrou commented on a change in pull request #11898:
URL: https://github.com/apache/arrow/pull/11898#discussion_r769658531
##########
File path: r/tests/testthat/test-data-type.R
##########
@@ -386,20 +386,34 @@ test_that("DictionaryType validation", {
test_that("decimal type and validation", {
expect_r6_class(decimal(4, 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')
+ 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")
+
+ # decimal() creates either decimal128 or decimal256 based on precision
+ expect_identical(class(decimal(38, 2)), class(decimal128(38, 2)))
+ expect_identical(class(decimal(39, 2)), class(decimal256(38, 2)))
- # decimal() is just an alias for decimal128() for backwards compatibility
expect_r6_class(decimal128(4, 2), "Decimal128Type")
- expect_identical(class(decimal(2, 4)), class(decimal128(2, 4)))
- expect_error(decimal128("four"), '"precision" must be an integer')
- expect_error(decimal128(4, "two"), '"scale" must be an integer')
- expect_error(decimal128(NA, 2), '"precision" must be an integer')
- expect_error(decimal128(4, NA), '"scale" must be an integer')
+ expect_error(decimal128("four"), "`precision` must be an integer")
+ expect_error(decimal128(4, "two"), "`scale` must be an integer")
+ expect_error(decimal128(NA, 2), "`precision` must be an integer")
+ 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")
+
+ expect_r6_class(decimal256(4, 2), "Decimal256Type")
+
+ expect_error(decimal256("four"), "`precision` must be an integer")
+ expect_error(decimal256(4, "two"), "`scale` must be an integer")
+ expect_error(decimal256(NA, 2), "`precision` must be an integer")
+ 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")
Review comment:
Can you also test what happens if the `precision` is too large for the
given type?
##########
File path: r/src/array_to_vector.cpp
##########
@@ -919,8 +920,9 @@ class Converter_Decimal : public Converter {
Status Ingest_some_nulls(SEXP data, const std::shared_ptr<arrow::Array>&
array,
R_xlen_t start, R_xlen_t n, size_t chunk_index)
const {
+ using DecimalArray = typename TypeTraits<Type>::ArrayType;
auto p_data = REAL(data) + start;
- const auto& decimals_arr = checked_cast<const
arrow::Decimal128Array&>(*array);
+ const auto& decimals_arr = checked_cast<const DecimalArray&>(*array);
auto ingest_one = [&](R_xlen_t i) {
p_data[i] = std::stod(decimals_arr.FormatValue(i).c_str());
Review comment:
Unrelated, but this looks quite inefficient (it roundtrips through a
string representation instead of creating a `double` directly). Can you open a
JIRA for this?
##########
File path: r/R/array.R
##########
@@ -187,8 +187,27 @@ Array$create <- function(x, type = NULL) {
}
return(out)
}
- vec_to_Array(x, type)
+
+ if (is.null(type)) {
+ return(vec_to_Array(x, type))
+ }
+
+ # a type is given, this needs more care
Review comment:
I think it would be nice to make the comment more precise about what
happens below.
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
