Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions r/src/r_to_arrow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -727,9 +727,48 @@ class RPrimitiveConverter<T, enable_if_t<is_timestamp_type<T>::value>>
template <typename T>
class RPrimitiveConverter<T, enable_if_t<is_decimal_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
using ValueType = typename arrow::TypeTraits<T>::CType;

public:
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
return Status::NotImplemented("Extend");
RETURN_NOT_OK(this->Reserve(size - offset));
int32_t precision = this->primitive_type_->precision();
int32_t scale = this->primitive_type_->scale();

auto append_value = [this, precision, scale](double value) {
ARROW_ASSIGN_OR_RAISE(ValueType converted,
ValueType::FromReal(value, precision, scale));
this->primitive_builder_->UnsafeAppend(converted);
return Status::OK();
};

auto append_null = [this]() {
this->primitive_builder_->UnsafeAppendNull();
return Status::OK();
};

switch (TYPEOF(x)) {
case REALSXP:
if (ALTREP(x)) {
return VisitVector(RVectorIterator_ALTREP<double>(x, offset), size, append_null,
append_value);
} else {
return VisitVector(RVectorIterator<double>(x, offset), size, append_null,
append_value);
}
break;
case INTSXP:
if (ALTREP(x)) {
return VisitVector(RVectorIterator_ALTREP<int>(x, offset), size, append_null,
append_value);
} else {
return VisitVector(RVectorIterator<int>(x, offset), size, append_null,
append_value);
}
break;
default:
return Status::NotImplemented("Conversion to decimal from non-integer/double");
}
}
};

Expand Down Expand Up @@ -1270,7 +1309,7 @@ std::shared_ptr<arrow::ChunkedArray> vec_to_arrow_ChunkedArray(
if (can_convert_native(x) && type->id() != Type::EXTENSION) {
// short circuit if `x` is an altrep vector that shells a chunked Array
auto maybe = altrep::vec_to_arrow_altrep_bypass(x);
if (maybe.get()) {
if (maybe.get() && maybe->type()->Equals(type)) {
return maybe;
}

Expand Down
61 changes: 50 additions & 11 deletions r/tests/testthat/test-Array.R
Original file line number Diff line number Diff line change
Expand Up @@ -869,17 +869,6 @@ test_that("Array$create() should have helpful error", {
expect_error(Array$create(list(lgl, lgl, int)), "Expecting a logical vector")
expect_error(Array$create(list(char, num, char)), "Expecting a character vector")

# hint at casting if direct fails and casting looks like it might work
expect_error(
Array$create(as.double(1:10), type = decimal(4, 2)),
"You might want to try casting manually"
)

expect_error(
Array$create(1:10, type = decimal(12, 2)),
"You might want to try casting manually"
)

a <- expect_error(Array$create("one", int32()))
b <- expect_error(vec_to_Array("one", int32()))
# the captured conditions (errors) are not identical, but their messages should be
Expand Down Expand Up @@ -1321,3 +1310,53 @@ test_that("Array to C-interface", {
delete_arrow_schema(schema_ptr)
delete_arrow_array(array_ptr)
})

test_that("Can convert R integer/double to decimal (ARROW-11631)", {
# Check both decimal128 and decimal256
decimal128_from_dbl <- Array$create(c(1, NA_real_), type = decimal128(12, 2))
decimal256_from_dbl <- Array$create(c(1, NA_real_), type = decimal256(12, 2))
decimal128_from_int <- Array$create(c(1L, NA_integer_), type = decimal128(12, 2))
decimal256_from_int <- Array$create(c(1L, NA_integer_), type = decimal256(12, 2))

# Check ALTREP input
altrep_dbl <- as.vector(Array$create(c(1, NA_real_)))
altrep_int <- as.vector(Array$create(c(1L, NA_integer_)))
decimal_from_altrep_dbl <- Array$create(altrep_dbl, type = decimal128(12, 2))
decimal_from_altrep_int <- Array$create(altrep_int, type = decimal128(12, 2))

expect_equal(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there's a branch for ALTREP, maybe decimal_array2 could be Array$create(1:10, type = decimal128(10, 2))?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(In trying my own example here I'm realizing that I didn't consider integer arrays...hang tight and I'll add it to the C++!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting...I now get this result:

> Array$create(c(1:10, NA))$cast(decimal128(10, 2))
Error: Invalid: Precision is not great enough for the result. It should be at least 12
/home/nic2/arrow/cpp/src/arrow/compute/exec.cc:828  kernel_->exec(kernel_ctx_, input, out)
/home/nic2/arrow/cpp/src/arrow/compute/exec.cc:796  ExecuteSingleSpan(input, &output)
/home/nic2/arrow/cpp/src/arrow/compute/function.cc:276  executor->Execute(input, &listener)

Looks like a bug given 10 significant digits is more than enough to represent the integers 1 through 10

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update the tests to use 12 and open a ticket

decimal128_from_dbl,
Array$create(c(1, NA))$cast(decimal128(12, 2))
)

expect_equal(
decimal256_from_dbl,
Array$create(c(1, NA))$cast(decimal256(12, 2))
)

expect_equal(
decimal128_from_int,
Array$create(c(1, NA))$cast(decimal128(12, 2))
)

expect_equal(
decimal256_from_int,
Array$create(c(1, NA))$cast(decimal256(12, 2))
)

expect_equal(
decimal_from_altrep_dbl,
Array$create(c(1, NA))$cast(decimal128(12, 2))
)

expect_equal(
decimal_from_altrep_int,
Array$create(c(1, NA))$cast(decimal128(12, 2))
)

# Check that other types aren't silently but invalidly converted
expect_error(
Array$create(complex(), decimal128(12, 2)),
"Conversion to decimal from non-integer/double"
)
})