Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-14844 [R] Implement decimal256() #11805

Closed
wants to merge 16 commits into from

Conversation

dragosmg
Copy link
Contributor

No description provided.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

Copy link
Contributor

@romainfrancois romainfrancois left a comment

Choose a reason for hiding this comment

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

in addition: dragosmg#1

@@ -387,6 +389,22 @@ decimal <- function(precision, scale) {
Decimal128Type__initialize(precision, scale)
}

#' @rdname data-type
#' @export
decimal256 <- function(precision, scale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps decimal() and decimal256 could call the same function for argument checking, e.g.

check_decimal_args <- function(precision, scale) {
  precision <- vec_cast(precision, to = integer())
  vec_assert(precision, size = 1L)

  scale <- vec_cast(scale, to = integer())
  vec_assert(scale, size = 1L)

  list(precision = precision, scale = scale)
}

#' @rdname data-type
#' @export
decimal <- function(precision, scale) {
  args <- check_decimal_args(precision, scale)
  Decimal128Type__initialize(args$precision, args$scale)
}

#' @rdname data-type
#' @export
decimal256 <- function(precision, scale) {
  args <- check_decimal_args(precision, scale)
  Decimal256Type__initialize(args$precision, args$scale)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! I will implement it.

@romainfrancois
Copy link
Contributor

As with decimal128 this would not allow for creating arrays directly:

library(arrow, warn.conflicts = FALSE)
#> See arrow_info() for available features

Array$create(c(1, 2), type = decimal(4, 3))
#> Error: NotImplemented: Extend

Created on 2021-12-03 by the reprex package (v2.0.1.9000)

The error comes from this class in r_to_arrow.cpp

template <typename T>
class RPrimitiveConverter<T, enable_if_t<is_decimal_type<T>::value>>
    : public PrimitiveConverter<T, RConverter> {
 public:
  Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
    return Status::NotImplemented("Extend");
  }
};

Maybe we could catch the NotImplemented error, then convert to a guessed type, and then cast.

@romainfrancois
Copy link
Contributor

Perhaps we could have something like this on the R side:

Array$create <- function(x, type = NULL) {
  if (!is.null(type)) {
    type <- as_type(type)
  }
  if (inherits(x, "Scalar")) {
    out <- x$as_array()
    if (!is.null(type)) {
      out <- out$cast(type)
    }
    return(out)
  }
  tryCatch(
    vec_to_Array(x, type),
    error = function(cnd) {
      if (!is.null(type)) {
        # try again and then cast
        vec_to_Array(x, NULL)$cast(type)
      } else {
        signalCondition(cnd)
      }
    }
  )
}

So that:

library(arrow, warn.conflicts = FALSE)
#> See arrow_info() for available features

Array$create(c(1, 2), type = decimal(4, 3))
#> Array
#> <decimal128(4, 3)>
#> [
#>   1.000,
#>   2.000
#> ]

Created on 2021-12-03 by the reprex package (v2.0.1.9000)

@dragosmg dragosmg closed this Dec 8, 2021
@dragosmg
Copy link
Contributor Author

dragosmg commented Dec 8, 2021

This is outdated. Work on decimal256() is taking place in a different PR - #11898

jonkeane pushed a commit that referenced this pull request Dec 20, 2021
@jonkeane & @romainfrancois this is the 2nd attempt at implementing `decimal256()`. First one is #11805

Closes #11898 from dragosmg/ARROW-14844_decimal256_take2

Lead-authored-by: Dragos Moldovan-Grünfeld <dragos.mold@gmail.com>
Co-authored-by: Dragoș Moldovan-Grünfeld <dragos.mold@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
rafael-telles pushed a commit to rafael-telles/arrow that referenced this pull request Dec 20, 2021
@jonkeane & @romainfrancois this is the 2nd attempt at implementing `decimal256()`. First one is apache#11805

Closes apache#11898 from dragosmg/ARROW-14844_decimal256_take2

Lead-authored-by: Dragos Moldovan-Grünfeld <dragos.mold@gmail.com>
Co-authored-by: Dragoș Moldovan-Grünfeld <dragos.mold@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
@dragosmg dragosmg deleted the ARROW-14844_decimal256 branch February 24, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants