[GitHub] [arrow] bkietz commented on a change in pull request #7279: ARROW-8938: [R] Provide binding for arrow::compute::CallFunction

2020-05-27 Thread GitBox 


bkietz commented on a change in pull request #7279:
URL: https://github.com/apache/arrow/pull/7279#discussion_r431417510



##
File path: r/src/compute.cpp
##
@@ -206,60 +206,91 @@ std::shared_ptr Table__FilterChunked(
   return tab;
 }
 
+template 
+std::shared_ptr MaybeUnbox(const char* class_name, SEXP x) {
+  if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, class_name)) {
+Rcpp::ConstReferenceSmartPtrInputParameter> obj(x);
+return static_cast>(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> 
obj(x);
-return static_cast>(obj);
-  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "ChunkedArray")) {
-
Rcpp::ConstReferenceSmartPtrInputParameter>
 obj(x);
-return static_cast>(obj);
-  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "RecordBatch")) {
-
Rcpp::ConstReferenceSmartPtrInputParameter> 
obj(x);
-return static_cast>(obj);
-  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "Table")) {
-Rcpp::ConstReferenceSmartPtrInputParameter> 
obj(x);
-return static_cast>(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("Array", x)) {
+return array;
+  }
+
+  if (auto chunked_array = MaybeUnbox("ChunkedArray", x)) 
{
+return chunked_array;
+  }
+
+  if (auto batch = MaybeUnbox("RecordBatch", x)) {
+return batch;
+  }
+
+  if (auto table = MaybeUnbox("Table", x)) {
+return table;
+  }
+
+  if (auto scalar = MaybeUnbox("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:
   Alright





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

[GitHub] [arrow] bkietz commented on a change in pull request #7279: ARROW-8938: [R] Provide binding for arrow::compute::CallFunction

2020-05-27 Thread GitBox 


bkietz commented on a change in pull request #7279:
URL: https://github.com/apache/arrow/pull/7279#discussion_r431273570



##
File path: r/src/compute.cpp
##
@@ -223,4 +205,66 @@ std::shared_ptr Table__FilterChunked(
   }
   return tab;
 }
+
+arrow::Datum to_datum(SEXP x) {

Review comment:
   How's this:
   ```c++
   template 
   std::shared_ptr MaybeUnbox(const char* class_name, SEXP x) {
 if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, class_name)) {
   Rcpp::ConstReferenceSmartPtrInputParameter> obj(x);
   return static_cast>(obj);
 }
 return nullptr;
   }
   
   arrow::Datum to_datum(SEXP x) {
 if (auto array = MaybeUnbox("Array", x)) {
   return array;
 }
   
 if (auto chunked_array = MaybeUnbox("ChunkedArray", 
x)) {
   return chunked_array;
 }
   
 // ...
   ```





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

[GitHub] [arrow] bkietz commented on a change in pull request #7279: ARROW-8938: [R] Provide binding for arrow::compute::CallFunction

2020-05-27 Thread GitBox 


bkietz commented on a change in pull request #7279:
URL: https://github.com/apache/arrow/pull/7279#discussion_r431266235



##
File path: r/src/compute.cpp
##
@@ -223,4 +205,66 @@ std::shared_ptr Table__FilterChunked(
   }
   return tab;
 }
+
+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> 
obj(x);
+return static_cast>(obj);
+  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "ChunkedArray")) {
+
Rcpp::ConstReferenceSmartPtrInputParameter>
 obj(x);
+return static_cast>(obj);
+  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "RecordBatch")) {
+
Rcpp::ConstReferenceSmartPtrInputParameter> 
obj(x);
+return static_cast>(obj);
+  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "Table")) {
+Rcpp::ConstReferenceSmartPtrInputParameter> 
obj(x);
+return static_cast>(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");
+  }
+}
+
+SEXP from_datum(arrow::Datum datum) {
+  if (datum.is_array()) {
+return Rcpp::wrap(datum.make_array());
+  } else if (datum.is_arraylike()) {
+return Rcpp::wrap(datum.chunked_array());
+  } else {
+// TODO: the other datum types
+Rcpp::stop("from_datum: Not implemented");
+  }
+}
+
+std::shared_ptr 
make_compute_options(std::string func_name,
+List_ options) {
+  if (func_name == "filter") {
+auto out = 
std::make_shared(arrow::compute::FilterOptions::Defaults());
+if (!Rf_isNull(options["keep_na"]) && options["keep_na"]) {
+  out->null_selection_behavior = arrow::compute::FilterOptions::EMIT_NULL;
+}
+return out;
+  } else if (func_name == "take") {
+auto out = 
std::make_shared(arrow::compute::TakeOptions::Defaults());
+return out;
+  } else {
+return nullptr;
+  }
+  // TODO: make sure the correct destructor is called?

Review comment:
   ```suggestion
   ```
   confirmed: `make_shared` sets the destructor to `deleter` (so we 
aren't relying on `~FunctionOptions` being virtual)





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

[GitHub] [arrow] bkietz commented on a change in pull request #7279: ARROW-8938: [R] Provide binding for arrow::compute::CallFunction

2020-05-27 Thread GitBox 


bkietz commented on a change in pull request #7279:
URL: https://github.com/apache/arrow/pull/7279#discussion_r431263416



##
File path: r/src/compute.cpp
##
@@ -223,4 +205,66 @@ std::shared_ptr Table__FilterChunked(
   }
   return tab;
 }
+
+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> 
obj(x);
+return static_cast>(obj);
+  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "ChunkedArray")) {
+
Rcpp::ConstReferenceSmartPtrInputParameter>
 obj(x);
+return static_cast>(obj);
+  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "RecordBatch")) {
+
Rcpp::ConstReferenceSmartPtrInputParameter> 
obj(x);
+return static_cast>(obj);
+  } else if (Rf_inherits(x, "ArrowObject") && Rf_inherits(x, "Table")) {
+Rcpp::ConstReferenceSmartPtrInputParameter> 
obj(x);
+return static_cast>(obj);
+  } else {
+// TODO: scalar?

Review comment:
   I would recommend refactoring that function into 
`std::shared_ptr to_scalar(SEXP)`. That function need not be exported 
(same logic as `Datum`; `SEXP` already holds whatever value). The climax of 
`to_scalar` should be a call to `arrow::MakeScalar`.
   
   After this:
   ```c++
   std::shared_ptr dataset___expr__scalar(SEXP x) {
 return ds::scalar(to_scalar(x));
   }
   ```





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