westonpace commented on code in PR #15211:
URL: https://github.com/apache/arrow/pull/15211#discussion_r1067044008
##########
r/src/r_to_arrow.cpp:
##########
@@ -727,9 +727,33 @@ 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>::BuilderType::ValueType;
+
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) {
+ auto converted = ValueType::FromReal(value, precision, scale);
+ RETURN_NOT_OK(converted);
+ this->primitive_builder_->UnsafeAppend(converted.ValueUnsafe());
Review Comment:
```suggestion
ARROW_ASSIGN_OR_RAISE(ValueType converted, ValueType::FromReal(value,
precision, scale));
this->primitive_builder_->UnsafeAppend(converted);
```
You can use `ARROW_ASSIGN_OR_RAISE` here. Also, I'm personally not a huge
fan of auto but YMMV.
##########
r/src/r_to_arrow.cpp:
##########
@@ -727,9 +727,33 @@ 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>::BuilderType::ValueType;
Review Comment:
```suggestion
using ValueType = typename arrow::TypeTraits<T>::CType;
```
I think this will work also and is a bit more direct.
##########
r/tests/testthat/test-Array.R:
##########
@@ -1321,3 +1321,19 @@ test_that("Array to C-interface", {
delete_arrow_schema(schema_ptr)
delete_arrow_array(array_ptr)
})
+
+test_that("direct creation of Decimal Arrays (ARROW-11631)", {
+
+ decimal_array <- Array$create(1, type = decimal128(10, 2))
+ decimal_array2 <- Array$create(1, type = decimal256(10, 2))
Review Comment:
> The purpose of decimal_array2 isn't clear to me here...am I missing
something?
Didn't you write this? My interpretation is that `decimal_array2` is meant
to verify the `decimal256` type works (in addition to the 128 bit variant).
> Could the test Array include an NA and something whose precision might get
truncated (i.e, maybe Array$create(c(1, 1 / 3, NA), type = decimal128(10, 2)))?
I think truncation is less interesting to me than NA. Testing truncation
would just be verifying that `FromReal` works correctly and we can assume it
does (or, if we have concerns, should test it elsewhere). Testing `NA` will
test that R's `NA` is getting properly recognized (e.g. adding coverage for
`append_null`).
##########
r/tests/testthat/test-Array.R:
##########
@@ -1321,3 +1321,19 @@ test_that("Array to C-interface", {
delete_arrow_schema(schema_ptr)
delete_arrow_array(array_ptr)
})
+
+test_that("direct creation of Decimal Arrays (ARROW-11631)", {
Review Comment:
I'm a little confused. I expected to see an R array of doubles get
converted to an Arrow array. I didn't expect to see decimals get created
directly.
--
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
