nealrichardson commented on a change in pull request #7279: URL: https://github.com/apache/arrow/pull/7279#discussion_r431404447
########## File path: r/R/scalar.R ########## @@ -0,0 +1,53 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +#' @include arrowExports.R + +#' Arrow scalars +#' +#' @description +#' `Scalar`s are used to store a singular value of an arrow `DataType` +#' +#' @name Scalar +#' @rdname Scalar +#' @export +Scalar <- R6Class("Scalar", inherit = ArrowObject, + public = list( + ToString = function() Scalar__ToString(self), + as_vector = function() Scalar__as_vector(self) + ), + active = list( + is_valid = function() Scalar__is_valid(self), + type = function() DataType$create(Scalar__type(self)) + ) +) +Scalar$create <- function(x) { + # TODO: it would probably be best if an explicit type could be provided + if (!inherits(x, "externalptr")) { + x <- Scalar__create(x) + } + shared_ptr(Scalar, x) +} + +#' @export +length.Scalar <- function(x) 1 Review comment: ```suggestion length.Scalar <- function(x) 1L ``` `length` should be an explicit integer, for consistency ########## File path: r/src/expression.cpp ########## @@ -104,57 +104,9 @@ std::shared_ptr<ds::Expression> dataset___expr__is_valid( // [[arrow::export]] std::shared_ptr<ds::Expression> dataset___expr__scalar(SEXP x) { - switch (TYPEOF(x)) { - case NILSXP: - return ds::scalar(std::make_shared<arrow::NullScalar>()); - case LGLSXP: - return ds::scalar(Rf_asLogical(x)); - case REALSXP: - if (Rf_inherits(x, "Date")) { - return ds::scalar(std::make_shared<arrow::Date32Scalar>(REAL(x)[0])); - } else if (Rf_inherits(x, "POSIXct")) { - return ds::scalar(std::make_shared<arrow::TimestampScalar>( - REAL(x)[0], arrow::timestamp(arrow::TimeUnit::SECOND))); - } else if (Rf_inherits(x, "integer64")) { - int64_t value = *reinterpret_cast<int64_t*>(REAL(x)); - return ds::scalar(value); - } else if (Rf_inherits(x, "difftime")) { - int multiplier = 0; - // TODO: shared with TimeConverter<> in array_from_vector.cpp - std::string unit(CHAR(STRING_ELT(Rf_getAttrib(x, arrow::r::symbols::units), 0))); - if (unit == "secs") { - multiplier = 1; - } else if (unit == "mins") { - multiplier = 60; - } else if (unit == "hours") { - multiplier = 3600; - } else if (unit == "days") { - multiplier = 86400; - } else if (unit == "weeks") { - multiplier = 604800; - } else { - Rcpp::stop("unknown difftime unit"); - } - return ds::scalar(std::make_shared<arrow::Time32Scalar>( - static_cast<int>(REAL(x)[0] * multiplier), - arrow::time32(arrow::TimeUnit::SECOND))); - } - return ds::scalar(Rf_asReal(x)); - case INTSXP: - if (Rf_inherits(x, "factor")) { - // TODO: This does not use the actual value, just the levels - auto type = arrow::r::InferArrowTypeFromFactor(x); - return ds::scalar(std::make_shared<arrow::DictionaryScalar>(type)); - } - return ds::scalar(Rf_asInteger(x)); - case STRSXP: - return ds::scalar(CHAR(STRING_ELT(x, 0))); - default: - Rcpp::stop( - tfm::format("R object of type %s not supported", Rf_type2char(TYPEOF(x)))); - } - - return nullptr; + // defined in scalar.cpp + std::shared_ptr<arrow::Scalar> Scalar__create(SEXP x); Review comment: Does this need to be added to the header file that is included here? I seem to recall having to do that before. ########## File path: r/R/scalar.R ########## @@ -0,0 +1,53 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +#' @include arrowExports.R + +#' Arrow scalars +#' +#' @description +#' `Scalar`s are used to store a singular value of an arrow `DataType` +#' +#' @name Scalar +#' @rdname Scalar +#' @export +Scalar <- R6Class("Scalar", inherit = ArrowObject, + public = list( + ToString = function() Scalar__ToString(self), + as_vector = function() Scalar__as_vector(self) + ), + active = list( + is_valid = function() Scalar__is_valid(self), + type = function() DataType$create(Scalar__type(self)) + ) +) +Scalar$create <- function(x) { + # TODO: it would probably be best if an explicit type could be provided Review comment: Can you cast a scalar? (Presumably so.) If so, you could add a `type = NULL` arg and `if (!is.null(type))` cast to that. Or, let the caller cast outside this method. ########## File path: r/src/compute.cpp ########## @@ -206,60 +206,91 @@ std::shared_ptr<arrow::Table> Table__FilterChunked( return tab; } +template <typename T> +std::shared_ptr<T> MaybeUnbox(const char* class_name, SEXP x) { + if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, class_name)) { + Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<T>> obj(x); + return static_cast<std::shared_ptr<T>>(obj); + } + return nullptr; +} + arrow::Datum to_datum(SEXP x) { - // TODO: this is repetitive, can we DRY it out? - if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "Array")) { - Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<arrow::Array>> obj(x); - return static_cast<std::shared_ptr<arrow::Array>>(obj); - } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "ChunkedArray")) { - Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<arrow::ChunkedArray>> obj(x); - return static_cast<std::shared_ptr<arrow::ChunkedArray>>(obj); - } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "RecordBatch")) { - Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<arrow::RecordBatch>> obj(x); - return static_cast<std::shared_ptr<arrow::RecordBatch>>(obj); - } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "Table")) { - Rcpp::ConstReferenceSmartPtrInputParameter<std::shared_ptr<arrow::Table>> obj(x); - return static_cast<std::shared_ptr<arrow::Table>>(obj); - } else { - // TODO: scalar? - // This assumes that R objects have already been converted to Arrow objects; - // that seems right but should we do the wrapping here too/instead? - Rcpp::stop("to_datum: Not implemented"); + if (auto array = MaybeUnbox<arrow::Array>("Array", x)) { + return array; + } + + if (auto chunked_array = MaybeUnbox<arrow::ChunkedArray>("ChunkedArray", x)) { + return chunked_array; + } + + if (auto batch = MaybeUnbox<arrow::RecordBatch>("RecordBatch", x)) { + return batch; + } + + if (auto table = MaybeUnbox<arrow::Table>("Table", x)) { + return table; + } + + if (auto scalar = MaybeUnbox<arrow::Scalar>("Scalar", x)) { + return scalar; } + + // This assumes that R objects have already been converted to Arrow objects; + // that seems right but should we do the wrapping here too/instead? + Rcpp::stop("to_datum: Not implemented for type %s", Rf_type2char(TYPEOF(x))); Review comment: Let's add a test for this error message ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
