bkietz commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r516255170
##########
File path: r/src/compute.cpp
##########
@@ -23,14 +23,22 @@
#include <arrow/record_batch.h>
#include <arrow/table.h>
+namespace cpp11 {
+template <>
+inline std::string r6_class_name<arrow::compute::CastOptions>(
+ const std::shared_ptr<arrow::compute::CastOptions>& codec) {
Review comment:
```suggestion
const std::shared_ptr<arrow::compute::CastOptions>&) {
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -300,22 +297,65 @@ bool GetBoolOption(const std::string& name, bool
default_);
namespace cpp11 {
template <typename T>
-using enable_if_shared_ptr = typename std::enable_if<
- std::is_same<std::shared_ptr<typename T::element_type>, T>::value,
T>::type;
+std::string r6_class_name(const std::shared_ptr<T>& x);
Review comment:
Based on the way this is specialized below, it seems we should prefer a
trait struct instead of a function to make future extension easier.
```suggestion
struct r6_class_name;
```
Specialized like:
```c++
template <>
struct r6_class_name<arrow::Field> {
static const char* get(const Field&) { return "Field"; }
};
// ...
template <>
struct r6_class_name<arrow::Array> {
static const char* get(const Array& array) {
switch (array.type_id()) {
case arrow::Type::DICTIONARY:
return "DictionaryArray";
//...
}
}
};
```
##########
File path: r/src/csv.cpp
##########
@@ -22,9 +22,36 @@
#include <arrow/csv/reader.h>
#include <arrow/util/value_parsing.h>
+namespace cpp11 {
+template <>
+inline std::string r6_class_name<arrow::csv::ReadOptions>(
+ const std::shared_ptr<arrow::csv::ReadOptions>& codec) {
Review comment:
```suggestion
const std::shared_ptr<arrow::csv::ReadOptions>&) {
```
##########
File path: r/src/datatype.cpp
##########
@@ -20,86 +20,163 @@
#if defined(ARROW_R_WITH_ARROW)
#include <arrow/type.h>
-// [[arrow::export]]
-bool shared_ptr_is_null(SEXP xp) {
- return
reinterpret_cast<std::shared_ptr<void>*>(R_ExternalPtrAddr(xp))->get() ==
- nullptr;
-}
+namespace cpp11 {
+template <>
+std::string r6_class_name<arrow::DataType>(const
std::shared_ptr<arrow::DataType>& type) {
+ using arrow::Type;
+
+ switch (type->id()) {
+ case Type::NA:
+ return "Null";
+ case Type::BOOL:
+ return "Boolean";
+ case Type::UINT8:
+ return "UInt8";
+ case Type::UINT16:
+ return "UInt16";
+ case Type::UINT32:
+ return "UInt32";
+ case Type::UINT64:
+ return "UInt64";
+
+ case Type::INT8:
+ return "Int8";
+ case Type::INT16:
+ return "Int16";
+ case Type::INT32:
+ return "Int32";
+ case Type::INT64:
+ return "Int64";
+
+ case Type::HALF_FLOAT:
+ return "Float16";
+ case Type::FLOAT:
+ return "Float32";
+ case Type::DOUBLE:
+ return "Float64";
+
+ case Type::STRING:
+ return "Utf8";
+ case Type::LARGE_STRING:
+ return "LargeUtf8";
+
+ case Type::BINARY:
+ return "Binary";
+ case Type::FIXED_SIZE_BINARY:
+ return "FixedSizeBinary";
+ case Type::LARGE_BINARY:
+ return "LargeBinary";
+
+ case Type::DATE32:
+ return "Date32";
+ case Type::DATE64:
+ return "Date64";
+ case Type::TIMESTAMP:
+ return "Timestamp";
+
+ case Type::TIME32:
+ return "Time32";
+ case Type::TIME64:
+ return "Time64";
+
+ case Type::DECIMAL:
+ return "Decimal128Type";
+
+ case Type::LIST:
+ return "ListType";
+ case Type::LARGE_LIST:
+ return "LargeListType";
+ case Type::FIXED_SIZE_LIST:
+ return "FixedSizeListType";
+
+ case Type::STRUCT:
+ return "StructType";
+ case Type::DICTIONARY:
+ return "DictionaryType";
+
+ default:
+ break;
+ }
-// [[arrow::export]]
-bool unique_ptr_is_null(SEXP xp) {
- return
reinterpret_cast<std::unique_ptr<void>*>(R_ExternalPtrAddr(xp))->get() ==
- nullptr;
+ return "DataType";
+
+ // switch(names(Type)[self$id + 1],
+ //
+ // INTERVAL = stop("Type INTERVAL not implemented yet"),
+ // SPARSE_UNION = stop("Type SPARSE_UNION not implemented yet"),
+ // DENSE_UNION = stop("Type DENSE_UNION not implemented yet"),
+ // MAP = stop("Type MAP not implemented yet"),
+ // EXTENSION = stop("Type EXTENSION not implemented yet"),
+ // DURATION = stop("Type DURATION not implemented yet"),
Review comment:
IIUC, if an `IntervalType` is passed to R we will now get an opaque
`DataType` instead of an error message. Whatever the case, please add a comment
describing behavior if one of these type ids is encountered
##########
File path: r/src/arrow_cpp11.h
##########
@@ -300,22 +297,65 @@ bool GetBoolOption(const std::string& name, bool
default_);
namespace cpp11 {
template <typename T>
-using enable_if_shared_ptr = typename std::enable_if<
- std::is_same<std::shared_ptr<typename T::element_type>, T>::value,
T>::type;
+std::string r6_class_name(const std::shared_ptr<T>& x);
template <typename T>
-enable_if_shared_ptr<T> as_cpp(SEXP from) {
- return arrow::r::ExternalPtrInput<T>(from);
+SEXP to_r6(const std::shared_ptr<T>& x) {
+ if (x == nullptr) return R_NilValue;
+
+ auto r_class_name = cpp11::r6_class_name<T>(x);
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+ SEXP r6_class = Rf_install(r_class_name.c_str());
Review comment:
```suggestion
cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
SEXP r6_class = Rf_install(cpp11::r6_class_name<T>::get(*x));
```
##########
File path: r/R/type.R
##########
@@ -232,101 +190,95 @@ NestedType <- R6Class("NestedType", inherit = DataType)
#' timestamp("ms", timezone = "CEST")
#' time64("ns")
#' }
-int8 <- function() shared_ptr(Int8, Int8__initialize())
+int8 <- function() Int8__initialize()
Review comment:
```suggestion
int8 <- Int8__initialize
```
?
##########
File path: r/src/compute.cpp
##########
@@ -123,19 +123,19 @@ arrow::Datum as_cpp<arrow::Datum>(SEXP x) {
SEXP from_datum(arrow::Datum datum) {
Review comment:
I don't think an R6 class for `Datum` has value, since SEXP already
fills the role of a discriminated union of the members of Datum.
With the addition of `R6` I amend my earlier comment: `from_datum` probably
be replaced by `R6::R6(Datum)`
##########
File path: r/src/arrow_cpp11.h
##########
@@ -300,22 +297,65 @@ bool GetBoolOption(const std::string& name, bool
default_);
namespace cpp11 {
template <typename T>
Review comment:
Please add a verbose comment describing how and when this should be
specialized
##########
File path: r/src/arrow_cpp11.h
##########
@@ -300,22 +297,65 @@ bool GetBoolOption(const std::string& name, bool
default_);
namespace cpp11 {
template <typename T>
-using enable_if_shared_ptr = typename std::enable_if<
- std::is_same<std::shared_ptr<typename T::element_type>, T>::value,
T>::type;
+std::string r6_class_name(const std::shared_ptr<T>& x);
template <typename T>
-enable_if_shared_ptr<T> as_cpp(SEXP from) {
- return arrow::r::ExternalPtrInput<T>(from);
+SEXP to_r6(const std::shared_ptr<T>& x) {
+ if (x == nullptr) return R_NilValue;
+
+ auto r_class_name = cpp11::r6_class_name<T>(x);
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+ SEXP r6_class = Rf_install(r_class_name.c_str());
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, r6_class,
arrow::r::symbols::new_));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval in arrow::
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
}
+} // namespace cpp11
+
+namespace arrow {
+namespace r {
template <typename T>
-SEXP as_sexp(const std::shared_ptr<T>& ptr) {
- return cpp11::external_pointer<std::shared_ptr<T>>(new
std::shared_ptr<T>(ptr));
+cpp11::writable::list to_r_list(const std::vector<std::shared_ptr<T>>& x) {
+ auto as_sexp = [&](const std::shared_ptr<T>& t) { return cpp11::to_r6<T>(t);
};
+ return to_r_vector<cpp11::writable::list>(x, as_sexp);
}
+} // namespace r
+} // namespace arrow
+
+class R6 {
+ public:
+ template <typename T>
+ R6(const std::shared_ptr<T>& x) : data_(cpp11::to_r6<T>(x)) {}
+
+ template <typename T>
+ R6(std::unique_ptr<T> x) :
data_(cpp11::to_r6<T>(std::shared_ptr<T>(x.release()))) {}
Review comment:
```suggestion
R6(std::unique_ptr<T> x) : data_(cpp11::to_r6<T>(std::move(x))) {}
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -24,16 +24,6 @@
#include "./nameof.h"
-namespace cpp11 {
-
-template <typename T>
-SEXP as_sexp(const std::shared_ptr<T>& ptr);
-
-template <typename T>
-SEXP as_sexp(const std::vector<std::shared_ptr<T>>& vec);
-
-} // namespace cpp11
-
// TODO: move this include up once we can resolve this issue in cpp11
// https://github.com/apache/arrow/pull/7819#discussion_r471664878
Review comment:
```suggestion
```
----------------------------------------------------------------
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:
[email protected]
