You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/24 12:02:07 UTC
[GitHub] [arrow] romainfrancois opened a new pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
romainfrancois opened a new pull request #8256:
URL: https://github.com/apache/arrow/pull/8256
call_function() internally unbox to the right R6 class.
This probably needs some more work, e.g. not sure how to deal with this function:
```r
.guess_result_class <- function(arg) {
# HACK HACK HACK delete this when call_function returns an ArrowObject itself
if (inherits(arg, "ArrowObject")) {
return(class(arg)[1])
} else if (inherits(arg, "array_expression")) {
return(arg$result_class)
} else {
stop("Not implemented")
}
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r495128561
##########
File path: r/src/arrow_exports.h
##########
@@ -22,6 +22,7 @@
#include "./arrow_cpp11.h"
#if defined(ARROW_R_WITH_ARROW)
+#include <arrow/dataset/api.h>
Review comment:
This was because of these classes that were not available in the fw header:
```cpp
R6_HANDLE(arrow::dataset::DirectoryPartitioning, "DirectoryPartitioning")
R6_HANDLE(arrow::dataset::HivePartitioning, "HivePartitioning")
```
forward declaring them seems to do the trick.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725644439
@kszucs For some reason the `test-r-linux-as-cran` job (branch) isn't being created on crossbow. Unfortunately the crossbow output (e.g. https://github.com/apache/arrow/runs/1386912319?check_suite_focus=true) doesn't provide any information about it.
FWIW it is still being triggered on the nightly builds: https://github.com/ursa-labs/crossbow/branches/all?query=test-r-linux-as-cran
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698808913
``` r
library(tidyverse)
brio::read_lines("~/git/apache/arrow/r/src/arrowExports.cpp") %>%
str_subset("^std::[a-z]+_ptr<") %>%
str_remove(" .*$") %>%
unique()
#> [1] "std::shared_ptr<arrow::Array>"
#> [2] "std::shared_ptr<arrow::DataType>"
#> [3] "std::shared_ptr<arrow::ArrayData>"
#> [4] "std::shared_ptr<arrow::ChunkedArray>"
#> [5] "std::shared_ptr<arrow::Buffer>"
#> [6] "std::shared_ptr<arrow::util::Codec>"
#> [7] "std::shared_ptr<arrow::io::CompressedOutputStream>"
#> [8] "std::shared_ptr<arrow::io::CompressedInputStream>"
#> [9] "std::shared_ptr<arrow::compute::CastOptions>"
#> [10] "std::shared_ptr<arrow::RecordBatch>"
#> [11] "std::shared_ptr<arrow::Table>"
#> [12] "std::shared_ptr<arrow::csv::ReadOptions>"
#> [13] "std::shared_ptr<arrow::csv::ParseOptions>"
#> [14] "std::shared_ptr<arrow::csv::ConvertOptions>"
#> [15] "std::shared_ptr<arrow::csv::TableReader>"
#> [16] "std::shared_ptr<ds::ScannerBuilder>"
#> [17] "std::shared_ptr<arrow::Schema>"
#> [18] "std::shared_ptr<ds::Dataset>"
#> [19] "std::shared_ptr<ds::UnionDataset>"
#> [20] "std::shared_ptr<ds::InMemoryDataset>"
#> [21] "std::shared_ptr<ds::FileFormat>"
#> [22] "std::shared_ptr<fs::FileSystem>"
#> [23] "std::shared_ptr<ds::DatasetFactory>"
#> [24] "std::shared_ptr<ds::ParquetFileFormat>"
#> [25] "std::shared_ptr<ds::IpcFileFormat>"
#> [26] "std::shared_ptr<ds::CsvFileFormat>"
#> [27] "std::shared_ptr<ds::Partitioning>"
#> [28] "std::shared_ptr<ds::PartitioningFactory>"
#> [29] "std::shared_ptr<ds::Scanner>"
#> [30] "std::shared_ptr<arrow::Field>"
#> [31] "std::shared_ptr<ds::Expression>"
#> [32] "std::shared_ptr<arrow::ipc::feather::Reader>"
#> [33] "std::shared_ptr<fs::FileSelector>"
#> [34] "std::shared_ptr<arrow::io::InputStream>"
#> [35] "std::shared_ptr<arrow::io::RandomAccessFile>"
#> [36] "std::shared_ptr<arrow::io::OutputStream>"
#> [37] "std::shared_ptr<fs::LocalFileSystem>"
#> [38] "std::shared_ptr<fs::SubTreeFileSystem>"
#> [39] "std::shared_ptr<fs::S3FileSystem>"
#> [40] "std::shared_ptr<arrow::io::MemoryMappedFile>"
#> [41] "std::shared_ptr<arrow::io::ReadableFile>"
#> [42] "std::shared_ptr<arrow::io::BufferReader>"
#> [43] "std::shared_ptr<arrow::io::FileOutputStream>"
#> [44] "std::shared_ptr<arrow::io::BufferOutputStream>"
#> [45] "std::shared_ptr<arrow::json::ReadOptions>"
#> [46] "std::shared_ptr<arrow::json::ParseOptions>"
#> [47] "std::shared_ptr<arrow::json::TableReader>"
#> [48] "std::shared_ptr<arrow::MemoryPool>"
#> [49] "std::shared_ptr<arrow::ipc::MessageReader>"
#> [50] "std::shared_ptr<arrow::ipc::Message>"
#> [51] "std::shared_ptr<parquet::ArrowReaderProperties>"
#> [52] "std::shared_ptr<parquet::arrow::FileReader>"
#> [53] "std::shared_ptr<parquet::ArrowWriterProperties>"
#> [54] "std::shared_ptr<parquet::WriterPropertiesBuilder>"
#> [55] "std::shared_ptr<parquet::WriterProperties>"
#> [56] "std::shared_ptr<parquet::arrow::FileWriter>"
#> [57] "std::shared_ptr<arrow::RecordBatchReader>"
#> [58] "std::shared_ptr<arrow::ipc::RecordBatchFileReader>"
#> [59] "std::shared_ptr<arrow::ipc::RecordBatchWriter>"
#> [60] "std::shared_ptr<arrow::Scalar>"
```
<sup>Created on 2020-09-25 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-718009888
If that one fix isn't enough, https://github.com/apache/arrow/commit/883eb572bc64430307112895976ba79df10c8c7d is probably where the relevant code came in, so see if you see anything there that needs updating.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725644439
@kszucs For some reason the `test-r-linux-as-cran` job (branch) isn't being created on crossbow. Unfortunately the crossbow output (e.g. https://github.com/apache/arrow/runs/1386912319?check_suite_focus=true) doesn't provide any information about it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r495061853
##########
File path: r/src/arrow_exports.h
##########
@@ -22,6 +22,7 @@
#include "./arrow_cpp11.h"
#if defined(ARROW_R_WITH_ARROW)
+#include <arrow/dataset/api.h>
Review comment:
Why do you need this (big) header here now?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724985123
@nealrichardson @romainfrancois I've refactored `r6_class_name` to a trait. For simple cases it's no longer necessary to declare the R6 class name- we correctly guess that `arrow::Field` corresponds to "Field". I've also added comments explaining why it's sometimes necessary to override this default and how, let me know if you'd like them more verbose.
> How does it fail/error if you forget to add one
For a new c++ function `shared_ptr<arrow::Widget> MakeWidget()`, the default R6 class name for the return value will be "Widget". If no such R6 class exists, we'll [`stop()`](https://github.com/apache/arrow/pull/8256/files#diff-ef920fdae5124d20b341b9710adf8a33367cd2d24e08510913419ead9c8bb40fR304-R306) anytime MakeWidget() is called (because we don't know how to box its return value).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724995201
[Another sort of failure you might see:](https://github.com/apache/arrow/pull/8256/checks?check_run_id=1382178751#step:8:3439)
```
── 5. Failure: RecordBatchStreamReader / Writer (@test-record-batch-reader.R#31)
`writer` inherits from `RecordBatchWriter/ArrowObject/R6` not `RecordBatchStreamWriter`.
```
In R there are separate R6 classes `/RecordBatch(|Stream|File)Writer/` but in c++ there's only `RecordBatchWriter`. It does not provide any property which would tell you whether it's writing to a stream or a file so there's no way we can tell what the right R6 class would be. In this instance, it's a clue that we should probably delete the extra R6 classes (leaving only RecordBatchWriter) since they're not doing anything except holding a `$create`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698445894
> So in short we would create the R6 object in C++ and don't have to manipulate the external pointers in R ?
Ideally, right?
> We would retain the same signatures ?
Right, I think the most developer-friendly experience would be to pack the externalptr -> R6 logic into the as_sexp method. And yeah `..dispatch` would probably go away entirely because you'd directly instantiate the correct subclass; that dispatch is because the R side didn't know exactly which subclass it was getting returned.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-720361840
Last commit goes in that direction with:
```cpp
namespace cpp11 {
template <typename T>
std::string r6_class_name(const std::shared_ptr<T>& x) ;
template <typename T>
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;
}
}
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()))){}
R6(SEXP data) : data_(data){}
operator SEXP() const {
return data_;
}
private:
SEXP data_;
};
```
so that the functions "just" return `R6` and the the `R6` constructor finds the right R6 class name by calling one of the `r6_class_name` methods.
This however means defining many of these, most of them being trivial:
```cpp
template <> inline std::string r6_class_name<arrow::Field>(const std::shared_ptr<arrow::Field>& array_data) {
return "Field";
}
```
but some of them with some notion of dispatch:
```cpptemplate <> std::string r6_class_name<ds::Dataset>(const std::shared_ptr<ds::Dataset>& dataset) {
auto type_name = dataset->type_name();
if (type_name == "union") {
return "UnionDataset";
} else if (type_name == "filesystem") {
return "FileSystemDataset";
} else if(type_name == "in-memory"){
return "InMemoryDataset";
} else {
return "Dataset";
}
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r520708027
##########
File path: r/src/recordbatch.cpp
##########
@@ -285,7 +284,8 @@ std::shared_ptr<arrow::RecordBatch> RecordBatch__from_arrays(SEXP schema_sxp, SE
int64_t num_rows = 0;
StopIfNotOk(arrow::r::check_consistent_array_size(arrays, &num_rows));
- return arrow::RecordBatch::Make(schema, num_rows, arrays);
+ auto out = arrow::RecordBatch::Make(schema, num_rows, arrays);
+ return out;
Review comment:
Is this change needed?
##########
File path: r/src/buffer.cpp
##########
@@ -41,20 +41,25 @@ int64_t Buffer__size(const std::shared_ptr<arrow::Buffer>& buffer) {
// [[arrow::export]]
std::shared_ptr<arrow::Buffer> r___RBuffer__initialize(SEXP x) {
+ std::shared_ptr<arrow::Buffer> out;
Review comment:
NBD but I don't think you need any of these changes anymore
##########
File path: r/src/recordbatchwriter.cpp
##########
@@ -48,6 +50,7 @@ std::shared_ptr<arrow::ipc::RecordBatchWriter> ipc___RecordBatchFileWriter__Open
auto options = arrow::ipc::IpcWriteOptions::Defaults();
options.write_legacy_ipc_format = use_legacy_format;
options.metadata_version = metadata_version;
+
Review comment:
More diff you can clean up if you want
##########
File path: r/tests/testthat/test-read-record-batch.R
##########
@@ -34,7 +34,7 @@ test_that("RecordBatchFileWriter / RecordBatchFileReader roundtrips", {
stream <- FileOutputStream$create(tf)
writer <- RecordBatchFileWriter$create(stream, tab$schema)
- expect_is(writer, "RecordBatchFileWriter")
+ expect_is(writer, "RecordBatchWriter")
Review comment:
Are these test changes necessary?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725035841
It's puzzling to me why that failure isn't on every build, but I can reproduce it locally. The issue is:
```r
min.Array <- function(..., na.rm = FALSE) {
scalar_aggregate("min_max", ..., na.rm = na.rm)$GetFieldByName("min")
}
```
`scalar_aggregate("min_max", ...)` (aka compute::CallFunction) should return a `StructScalar` but instead it's returning just a `Scalar`, which doesn't have a `GetFieldByName` method.
There is the Scalar/StructScalar handling in scalar.cpp https://github.com/apache/arrow/pull/8256/files#diff-15908f875c08c0199f6416241e0aa043509eed9741ba6e5bcb6113940ec61105
But maybe (and on certain compilers only?) it's not getting picked up correctly in compute.cpp https://github.com/apache/arrow/pull/8256/files#diff-a138b87f0da58d824c72293eab62d9f352718dcb7f41a47ea7e1c3c84bbe27ddR136 ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698983439
started to get away from the R function `shared_ptr()`. Not handling all cases yet, eventually this:
```cpp
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));
}
```
will disappear and we'll have individual functions for each as generated by some macro.
I'm not clear yet about the classes that are currently handled with `$..dispatch()` methods in R.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724995201
Another sort of failure you might see: https://github.com/apache/arrow/pull/8256/checks?check_run_id=1382178751#step:8:3439
```
```
We have separate R6 classes `/RecordBatch(|Stream|File)Writer/` but in c++ there's only `RecordBatchWriter`. It does not provide any property which would tell you whether it's writing to a stream or a file, so
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725012165
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698302396
https://issues.apache.org/jira/browse/ARROW-9001
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-717997830
It's a test file that I added after you started this PR, and it's not run on every CI job, and you aren't running it locally unless you set up minio. See https://github.com/apache/arrow/blob/master/r/README.md#running-tests. On macOS you can `brew install minio`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494783594
##########
File path: r/R/compute.R
##########
@@ -127,15 +127,15 @@ match_arrow.Array <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- Array$create(call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
#' @export
match_arrow.ChunkedArray <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- shared_ptr(ChunkedArray, call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
Review comment:
We can go further when tackling this: https://issues.apache.org/jira/browse/ARROW-10089
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494402171
##########
File path: r/R/compute.R
##########
@@ -127,15 +127,15 @@ match_arrow.Array <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- Array$create(call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
#' @export
match_arrow.ChunkedArray <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- shared_ptr(ChunkedArray, call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
Review comment:
This can be `match_arrow.ChunkedArray <- match_arrow.Array` now since they're the same. There are probably a few others like this (I see `Ops` also, for example).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r495148711
##########
File path: r/src/arrow_exports.h
##########
@@ -22,6 +22,7 @@
#include "./arrow_cpp11.h"
#if defined(ARROW_R_WITH_ARROW)
+#include <arrow/dataset/api.h>
Review comment:
👍 We should probably add the forward declarations to arrow/dataset/type_fwd.h
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-699054273
I'd like to make the `..dispatch()` methods go away. So I guess the question is when a C++ function returns an Array that in fact is a StructArray, should the R object be Array or StructArray.
Now, with `..dispatch()` we do resolve them so that we get a StructArray
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494402171
##########
File path: r/R/compute.R
##########
@@ -127,15 +127,15 @@ match_arrow.Array <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- Array$create(call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
#' @export
match_arrow.ChunkedArray <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- shared_ptr(ChunkedArray, call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
Review comment:
This can be `match_arrow.ChunkedArray <- match_arrow.Array` now since they're the same. There are probably a few others like this (I see `Ops` also, for example).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698302396
https://issues.apache.org/jira/browse/ARROW-9001
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-718045112
This last commit is related to https://issues.apache.org/jira/browse/ARROW-10080 and gives a way to release memory immediately for RecordBatch and Table. If we want more classes (maybe Array etc ... ?) we can add them to generated code in `codegen.R`.
`$invalidate()` can be used on any arrow object but it only release the memory (i.e. call the destructor) if the method is generated:
```r
invalidate = function() {
cl <- class(self)[1L]
# if there is a Reset function for that class, call it
reset <- get(paste0("_arrow_", cl, "__Reset"), ns_arrow)
if (!is.null(reset)) {
get(".Call")(reset, self)
}
# but in any case, set the external pointer to NULL
assign(".:xp:.", NULL, envir = self)
}
```
otherwise it just sets the external pointer to NULL which is less useful because the memory will be reclaimed only later as part of a garbage collection.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698321096
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725012888
Revision: f5854420cf94d693b291c7ca29a164d0052113ab
Submitted crossbow builds: [ursa-labs/crossbow @ actions-700](https://github.com/ursa-labs/crossbow/branches/all?query=actions-700)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://travis-ci.org/ursa-labs/crossbow/branches)|
|test-r-linux-as-cran|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-700-github-test-r-linux-as-cran)|
|test-r-rhub-ubuntu-gcc-release|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rhub-ubuntu-gcc-release)|
|test-r-rocker-r-base-latest|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rocker-r-base-latest)|
|test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rstudio-r-base-3.6-bionic)|
|test-r-rstudio-r-base-3.6-centos6|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rstudio-r-base-3.6-centos6)|
|test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rstudio-r-base-3.6-centos8)|
|test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rstudio-r-base-3.6-opensuse15)|
|test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-r-rstudio-r-base-3.6-opensuse42)|
|test-ubuntu-18.04-r-sanitizer|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-700-azure-test-ubuntu-18.04-r-sanitizer)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725616996
Revision: 2c3dd4279cd5fd9d403bc8aa1f4e0ec3323d081c
Submitted crossbow builds: [ursa-labs/crossbow @ actions-701](https://github.com/ursa-labs/crossbow/branches/all?query=actions-701)
|Task|Status|
|----|------|
|homebrew-r-autobrew|[](https://travis-ci.org/ursa-labs/crossbow/branches)|
|test-r-linux-as-cran|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-701-github-test-r-linux-as-cran)|
|test-r-rhub-ubuntu-gcc-release|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rhub-ubuntu-gcc-release)|
|test-r-rocker-r-base-latest|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rocker-r-base-latest)|
|test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rstudio-r-base-3.6-bionic)|
|test-r-rstudio-r-base-3.6-centos6|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rstudio-r-base-3.6-centos6)|
|test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rstudio-r-base-3.6-centos8)|
|test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rstudio-r-base-3.6-opensuse15)|
|test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-r-rstudio-r-base-3.6-opensuse42)|
|test-ubuntu-18.04-r-sanitizer|[](https://dev.azure.com/ursa-labs/crossbow/_build/latest?definitionId=1&branchName=actions-701-azure-test-ubuntu-18.04-r-sanitizer)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494775017
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, symbol, fun_symbol));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval:
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
Review comment:
Perhaps this is more of a hypothetical `cpp11::call` thing, e.g.
```cpp
cpp11::call call(cpp11::call(R_DollarSymbol, symbol, fun_symbol), xp);
cpp11::sexp result = call.eval(arrow::r::ns::arrow);
```
Then I guess `cpp11::function` could factor out some of its logic in `cpp11::call`
##########
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:
Would that still do the dispatch internally before reaching the R side or would there be an R6 class for `arrow::Datum`. Probably the latter.
##########
File path: r/R/compute.R
##########
@@ -127,15 +127,15 @@ match_arrow.Array <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- Array$create(call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
#' @export
match_arrow.ChunkedArray <- function(x, table, ...) {
if (!inherits(table, c("Array", "ChunkedArray"))) {
table <- Array$create(table)
}
- shared_ptr(ChunkedArray, call_function("index_in_meta_binary", x, table))
+ call_function("index_in_meta_binary", x, table)
}
Review comment:
We can go further when tackling this: https://issues.apache.org/jira/browse/ARROW-10089
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
kou commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725660754
FYI: https://github.com/apache/arrow/pull/8386#issuecomment-725085209
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724985123
@nealrichardson @romainfrancois I've refactored `r6_class_name` to a trait. For simple cases it's no longer necessary to declare the R6 class name- we correctly guess that `arrow::Field` corresponds to "Field". I've also added comments explaining why it's sometimes necessary to override this default and how, let me know if you'd like them more verbose.
> How does it fail/error if you forget to add one
For a new c++ function `shared_ptr<arrow::Widget> MakeWidget()`, the default R6 class name for the return value will be "Widget". If no such R6 class exists, we'll [`stop()`](https://github.com/apache/arrow/pull/8256/files#diff-ef920fdae5124d20b341b9710adf8a33367cd2d24e08510913419ead9c8bb40fR304-R306) anytime MakeWidget() is called (because we don't know how to box its return value). For example, when I tried to change the return type of `dataset___UnionDatasetFactory__Make` I got:
```
══ Failed ══════════════════════════════════════════════════════════════════════
── 1. Error: Assembling multiple DatasetFactories with DatasetFactory (@test-dat
No arrow R6 class named 'UnionDatasetFactory'
Backtrace:
1. DatasetFactory$create(list(factory1, factory2)) testthat/test-dataset.R:723:2
2. arrow:::dataset___UnionDatasetFactory__Make(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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725616136
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494467875
##########
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:
Someday this should probably be
```c++
SEXP as_sexp(arrow::Datum datum)
```
so that we don't need to wrap datums in `from_datum`
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
Review comment:
```suggestion
// make call: <symbol>$<fun_symbol>(<x>)
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, symbol, fun_symbol));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval:
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
Review comment:
This seems like something we'd like to use cpp11::function for, but among other things that always calls the function in R_GlobalEnv
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, symbol, fun_symbol));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval:
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
+}
+
+template <typename T>
+SEXP R6_new(SEXP symbol, const std::shared_ptr<T>& x) {
+ return R6_make(symbol, arrow::r::symbols::new_, x);
+}
+
+template <typename T>
+SEXP R6_create(SEXP symbol, const std::shared_ptr<T>& x) {
+ return R6_make(symbol, arrow::r::symbols::create, x);
+}
Review comment:
This isn't shared_ptr specific; we could just write
```suggestion
template <typename T>
SEXP R6_new(SEXP symbol, T x) {
return R6_make(symbol, arrow::r::symbols::new_, std::move(x));
}
template <typename T>
SEXP R6_create(SEXP symbol, T x) {
return R6_make(symbol, arrow::r::symbols::create, std::move(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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698808913
``` r
library(tidyverse)
brio::read_lines("~/git/apache/arrow/r/src/arrowExports.cpp") %>%
str_subset("^std::[a-z]+_ptr<") %>%
str_remove(" .*$") %>%
unique()
#> [1] "std::shared_ptr<arrow::Array>"
#> [2] "std::shared_ptr<arrow::DataType>"
#> [3] "std::shared_ptr<arrow::ArrayData>"
#> [4] "std::shared_ptr<arrow::ChunkedArray>"
#> [5] "std::shared_ptr<arrow::Buffer>"
#> [6] "std::shared_ptr<arrow::util::Codec>"
#> [7] "std::shared_ptr<arrow::io::CompressedOutputStream>"
#> [8] "std::shared_ptr<arrow::io::CompressedInputStream>"
#> [9] "std::shared_ptr<arrow::compute::CastOptions>"
#> [10] "std::shared_ptr<arrow::RecordBatch>"
#> [11] "std::shared_ptr<arrow::Table>"
#> [12] "std::shared_ptr<arrow::csv::ReadOptions>"
#> [13] "std::shared_ptr<arrow::csv::ParseOptions>"
#> [14] "std::shared_ptr<arrow::csv::ConvertOptions>"
#> [15] "std::shared_ptr<arrow::csv::TableReader>"
#> [16] "std::shared_ptr<ds::ScannerBuilder>"
#> [17] "std::shared_ptr<arrow::Schema>"
#> [18] "std::shared_ptr<ds::Dataset>"
#> [19] "std::shared_ptr<ds::UnionDataset>"
#> [20] "std::shared_ptr<ds::InMemoryDataset>"
#> [21] "std::shared_ptr<ds::FileFormat>"
#> [22] "std::shared_ptr<fs::FileSystem>"
#> [23] "std::shared_ptr<ds::DatasetFactory>"
#> [24] "std::shared_ptr<ds::ParquetFileFormat>"
#> [25] "std::shared_ptr<ds::IpcFileFormat>"
#> [26] "std::shared_ptr<ds::CsvFileFormat>"
#> [27] "std::shared_ptr<ds::Partitioning>"
#> [28] "std::shared_ptr<ds::PartitioningFactory>"
#> [29] "std::shared_ptr<ds::Scanner>"
#> [30] "std::shared_ptr<arrow::Field>"
#> [31] "std::shared_ptr<ds::Expression>"
#> [32] "std::shared_ptr<arrow::ipc::feather::Reader>"
#> [33] "std::shared_ptr<fs::FileSelector>"
#> [34] "std::shared_ptr<arrow::io::InputStream>"
#> [35] "std::shared_ptr<arrow::io::RandomAccessFile>"
#> [36] "std::shared_ptr<arrow::io::OutputStream>"
#> [37] "std::shared_ptr<fs::LocalFileSystem>"
#> [38] "std::shared_ptr<fs::SubTreeFileSystem>"
#> [39] "std::shared_ptr<fs::S3FileSystem>"
#> [40] "std::shared_ptr<arrow::io::MemoryMappedFile>"
#> [41] "std::shared_ptr<arrow::io::ReadableFile>"
#> [42] "std::shared_ptr<arrow::io::BufferReader>"
#> [43] "std::shared_ptr<arrow::io::FileOutputStream>"
#> [44] "std::shared_ptr<arrow::io::BufferOutputStream>"
#> [45] "std::shared_ptr<arrow::json::ReadOptions>"
#> [46] "std::shared_ptr<arrow::json::ParseOptions>"
#> [47] "std::shared_ptr<arrow::json::TableReader>"
#> [48] "std::shared_ptr<arrow::MemoryPool>"
#> [49] "std::shared_ptr<arrow::ipc::MessageReader>"
#> [50] "std::shared_ptr<arrow::ipc::Message>"
#> [51] "std::shared_ptr<parquet::ArrowReaderProperties>"
#> [52] "std::shared_ptr<parquet::arrow::FileReader>"
#> [53] "std::shared_ptr<parquet::ArrowWriterProperties>"
#> [54] "std::shared_ptr<parquet::WriterPropertiesBuilder>"
#> [55] "std::shared_ptr<parquet::WriterProperties>"
#> [56] "std::shared_ptr<parquet::arrow::FileWriter>"
#> [57] "std::shared_ptr<arrow::RecordBatchReader>"
#> [58] "std::shared_ptr<arrow::ipc::RecordBatchFileReader>"
#> [59] "std::shared_ptr<arrow::ipc::RecordBatchWriter>"
#> [60] "std::shared_ptr<arrow::Scalar>"
```
<sup>Created on 2020-09-25 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698808913
``` r
library(tidyverse)
brio::read_lines("~/git/apache/arrow/r/src/arrowExports.cpp") %>%
str_subset("^std::[a-z]+_ptr<") %>%
str_remove(" .*$") %>%
unique() %>%
str_remove("^.*<") %>%
str_remove(">")
#> [1] "arrow::Array" "arrow::DataType"
#> [3] "arrow::ArrayData" "arrow::ChunkedArray"
#> [5] "arrow::Buffer" "arrow::util::Codec"
#> [7] "arrow::io::CompressedOutputStream" "arrow::io::CompressedInputStream"
#> [9] "arrow::compute::CastOptions" "arrow::RecordBatch"
#> [11] "arrow::Table" "arrow::csv::ReadOptions"
#> [13] "arrow::csv::ParseOptions" "arrow::csv::ConvertOptions"
#> [15] "arrow::csv::TableReader" "ds::ScannerBuilder"
#> [17] "arrow::Schema" "ds::Dataset"
#> [19] "ds::UnionDataset" "ds::InMemoryDataset"
#> [21] "ds::FileFormat" "fs::FileSystem"
#> [23] "ds::DatasetFactory" "ds::ParquetFileFormat"
#> [25] "ds::IpcFileFormat" "ds::CsvFileFormat"
#> [27] "ds::Partitioning" "ds::PartitioningFactory"
#> [29] "ds::Scanner" "arrow::Field"
#> [31] "ds::Expression" "arrow::ipc::feather::Reader"
#> [33] "fs::FileSelector" "arrow::io::InputStream"
#> [35] "arrow::io::RandomAccessFile" "arrow::io::OutputStream"
#> [37] "fs::LocalFileSystem" "fs::SubTreeFileSystem"
#> [39] "fs::S3FileSystem" "arrow::io::MemoryMappedFile"
#> [41] "arrow::io::ReadableFile" "arrow::io::BufferReader"
#> [43] "arrow::io::FileOutputStream" "arrow::io::BufferOutputStream"
#> [45] "arrow::json::ReadOptions" "arrow::json::ParseOptions"
#> [47] "arrow::json::TableReader" "arrow::MemoryPool"
#> [49] "arrow::ipc::MessageReader" "arrow::ipc::Message"
#> [51] "parquet::ArrowReaderProperties" "parquet::arrow::FileReader"
#> [53] "parquet::ArrowWriterProperties" "parquet::WriterPropertiesBuilder"
#> [55] "parquet::WriterProperties" "parquet::arrow::FileWriter"
#> [57] "arrow::RecordBatchReader" "arrow::ipc::RecordBatchFileReader"
#> [59] "arrow::ipc::RecordBatchWriter" "arrow::Scalar"
```
<sup>Created on 2020-09-25 by the [reprex package](https://reprex.tidyverse.org) (v0.3.0.9001)</sup>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698321096
Turns out we just don't need to keep track of the result_type anymore I think.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r513533285
##########
File path: r/R/filesystem.R
##########
@@ -285,6 +262,7 @@ FileSystem <- R6Class("FileSystem", inherit = ArrowObject,
# TODO: see fs package for glob/regexp filtering
# TODO: verbose method that shows other attributes as df
# TODO: print methods for FileInfo, SubTreeFileSystem, S3FileSystem
+ fs___FileSystem__OpenAppendStream(self, clean_path_rel(path))
Review comment:
Here's one of your bugs
```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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-720682103
Sounds promising, will take a look.
Test failures point to some S3 bindings that need updating too: https://github.com/apache/arrow/pull/8256/checks?check_run_id=1341403375#step:8:2938
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #8256:
URL: https://github.com/apache/arrow/pull/8256
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725615670
[ASAN UBSAN CI failure](https://github.com/apache/arrow/pull/8256/checks?check_run_id=1386783871#step:8:2353) is ARROW-10525
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494775017
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, symbol, fun_symbol));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval:
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
Review comment:
Perhaps this is more of a hypothetical `cpp11::call` thing, e.g.
```cpp
cpp11::call call(cpp11::call(R_DollarSymbol, symbol, fun_symbol), xp);
cpp11::sexp result = call.eval(arrow::r::ns::arrow);
```
Then I guess `cpp11::function` could factor out some of its logic in `cpp11::call`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r516761809
##########
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:
Is there any way this could get a default using `nameof()`? I think that'd cover most of the cases like `Field` which don't require dispatch
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494775915
##########
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:
Would that still do the dispatch internally before reaching the R side or would there be an R6 class for `arrow::Datum`. Probably the latter.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-719453483
> A few things I noticed, mostly rebase cleanup.
>
> It kinda feels like we've just moved the shared_ptr() business down a level since we still have to explicitly wrap and declare types for the output, though that is an improvement. I had envisioned something, either as as_sexp methods or as some custom wrapping in codegen.R, that would handle the mapping from the return signature (e.g. std::shared_ptr<arrow::Table>) to R6 class name (Table). Maybe that was wishful thinking.
Let me try to push this further.
Now all the functions return an `R6`, which for now is just an alias to `SEXP` but may become some other thing, and it's our job to make the right R6 based on the shared pointer we mean to wrap.
This is indeed just moving the needle and pain down on level.
What I can try to do is that `R6` be a class that:
- can be converted to `SEXP`, i.e. it has a `operator SEXP()`
- can be constructed from the various types, e.g. we'd have many constructors like `R6(const std::shared_ptr<T>&)` for all the `T` we handle, and some of these (e.g. `R6(const std::shared_ptr<arrow::Array>&)` do some dispatch (what `r6_Array()` does now).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725639997
@github-actions crossbow submit test-r-linux-as-cran
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r520380853
##########
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:
Oh interesting, I had not seen `nameof()` before.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725640851
Revision: 2c3dd4279cd5fd9d403bc8aa1f4e0ec3323d081c
Submitted crossbow builds: [ursa-labs/crossbow @ actions-702](https://github.com/ursa-labs/crossbow/branches/all?query=actions-702)
|Task|Status|
|----|------|
|test-r-linux-as-cran|[](https://github.com/ursa-labs/crossbow/actions?query=branch:actions-702-github-test-r-linux-as-cran)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724553352
Thanks @bkietz for the review, I'll work towards using a trait instead of a template function.
As per yesterday's discussion with @nealrichardson, we also want to get back at returning the "real" types from the C++ functions, as opposed to the catch all `R6`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r494467875
##########
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:
Someday this should probably be
```c++
SEXP as_sexp(arrow::Datum datum)
```
so that we don't need to wrap datums in `from_datum`
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
Review comment:
```suggestion
// make call: <symbol>$<fun_symbol>(<x>)
```
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, symbol, fun_symbol));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval:
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
Review comment:
This seems like something we'd like to use cpp11::function for, but among other things that always calls the function in R_GlobalEnv
##########
File path: r/src/arrow_cpp11.h
##########
@@ -310,4 +312,32 @@ enable_if_enum<E, SEXP> as_sexp(E e) {
return as_sexp(static_cast<int>(e));
}
+template <typename T>
+SEXP R6_make(SEXP symbol, SEXP fun_symbol, const std::shared_ptr<T>& x) {
+ if (x == nullptr) {
+ return R_NilValue;
+ }
+ cpp11::external_pointer<std::shared_ptr<T>> xp(new std::shared_ptr<T>(x));
+
+ // make call: <symbol>$new(<x>)
+ SEXP call = PROTECT(Rf_lang3(R_DollarSymbol, symbol, fun_symbol));
+ SEXP call2 = PROTECT(Rf_lang2(call, xp));
+
+ // and then eval:
+ SEXP r6 = PROTECT(Rf_eval(call2, arrow::r::ns::arrow));
+
+ UNPROTECT(3);
+ return r6;
+}
+
+template <typename T>
+SEXP R6_new(SEXP symbol, const std::shared_ptr<T>& x) {
+ return R6_make(symbol, arrow::r::symbols::new_, x);
+}
+
+template <typename T>
+SEXP R6_create(SEXP symbol, const std::shared_ptr<T>& x) {
+ return R6_make(symbol, arrow::r::symbols::create, x);
+}
Review comment:
This isn't shared_ptr specific; we could just write
```suggestion
template <typename T>
SEXP R6_new(SEXP symbol, T x) {
return R6_make(symbol, arrow::r::symbols::new_, std::move(x));
}
template <typename T>
SEXP R6_create(SEXP symbol, T x) {
return R6_make(symbol, arrow::r::symbols::create, std::move(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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-717815859
Finished rebasing this, now seeing, not sure what this is about:
```
> test_check("arrow")
3491
── 1. Error: write_parquet with filesystem arg (@test-s3-minio.R#102) ─────────
3492
NotImplemented: It is not possible to append efficiently to S3 objects
3493
Backtrace:
3494
1. testthat::expect_length(fs$ls(minio_path("hive_dir")), 2)
3495
4. fs$ls(minio_path("hive_dir"))
3496
5. arrow:::fs___FileSystem__OpenAppendStream(self, clean_path_rel(path))
3497
3498
── 2. Error: open_dataset with fs (@test-s3-minio.R#113) ──────────────────────
3499
Can't subset columns that don't exist.
3500
✖ Column `dbl` doesn't exist.
3501
Backtrace:
3502
1. testthat::expect_identical(...)
3503
9. dplyr::select(., dbl, lgl)
3504
13. arrow:::column_select(arrow_dplyr_query(.data), !!!enquos(...))
3505
14. tidyselect:::.FUN(names(.data), !!!enquos(...))
3506
15. tidyselect:::eval_select_impl(...)
3507
23. tidyselect:::vars_select_eval(...)
3508
24. tidyselect:::walk_data_tree(expr, data_mask, context_mask)
3509
25. tidyselect:::eval_c(expr, data_mask, context_mask)
3510
26. tidyselect:::reduce_sels(node, data_mask, context_mask, init = init)
3511
27. tidyselect:::walk_data_tree(new, data_mask, context_mask)
3512
28. tidyselect:::as_indices_sel_impl(...)
3513
29. tidyselect:::as_indices_impl(x, vars, strict = strict)
3514
30. tidyselect:::chr_as_locations(x, vars)
3515
31. vctrs::vec_as_location(x, n = length(vars), names = vars)
3516
33. vctrs:::stop_subscript_oob(...)
3517
34. vctrs:::stop_subscript(...)
3518
3519
── 3. Error: write_dataset with fs (@test-s3-minio.R#122) ─────────────────────
3520
IOPath does not exist '1603877944.93906/new_dataset_dir'
3521
Backtrace:
3522
1. testthat::expect_length(...)
3523
4. fs$ls(minio_path("new_dataset_dir"))
3524
5. self$GetFileInfo(selector)
3525
6. arrow:::fs___FileSystem__GetTargetInfos_FileSelector(self, x)
3526
3527
── 4. Error: Let's test copy_files too (@test-s3-minio.R#136) ─────────────────
3528
Can't subset columns that don't exist.
3529
✖ Column `dbl` doesn't exist.
3530
Backtrace:
3531
1. testthat::expect_identical(...)
3532
9. dplyr::select(., dbl, lgl)
3533
13. arrow:::column_select(arrow_dplyr_query(.data), !!!enquos(...))
3534
14. tidyselect:::.FUN(names(.data), !!!enquos(...))
3535
15. tidyselect:::eval_select_impl(...)
3536
23. tidyselect:::vars_select_eval(...)
3537
24. tidyselect:::walk_data_tree(expr, data_mask, context_mask)
3538
25. tidyselect:::eval_c(expr, data_mask, context_mask)
3539
26. tidyselect:::reduce_sels(node, data_mask, context_mask, init = init)
3540
27. tidyselect:::walk_data_tree(new, data_mask, context_mask)
3541
28. tidyselect:::as_indices_sel_impl(...)
3542
29. tidyselect:::as_indices_impl(x, vars, strict = strict)
3543
30. tidyselect:::chr_as_locations(x, vars)
3544
31. vctrs::vec_as_location(x, n = length(vars), names = vars)
3545
33. vctrs:::stop_subscript_oob(...)
3546
34. vctrs:::stop_subscript(...)
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698439528
> I love this and I want this everywhere :)
I sort of assumed you would go there, as I was working on #8246 too. So in short we would create the R6 object in C++ and don't have to manipulate the external pointers in R ? I can see the appeal, but I need to work out some logistics.
We would retain the same signatures ? e.g.
```cpp
// [[arrow::export]]
std::shared_ptr<arrow::DataType> Int8__initialize() { return arrow::int8(); }
```
presently before this goes to R, this is wrapped in the external pointer, so we have a `cpp11::external_pointer<std::shared_ptr<arrow::DataType>>`, then in the R side it gets wrapped into a (subclass of) `DataType`, but I suppose that we could have that the `std::shared_ptr<arrow::DataType>` is directly wrapped to the correct R6 instance or NULL, this is essentially moving the weird `..dispatch` method down C++ layer.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698445894
> So in short we would create the R6 object in C++ and don't have to manipulate the external pointers in R ?
Ideally, right?
> We would retain the same signatures ?
Right, I think the most developer-friendly experience would be to pack the externalptr -> R6 logic into the as_sexp method. And yeah `..dispatch` would probably go away entirely because you'd directly instantiate the correct subclass; that dispatch is because the R side didn't know exactly which subclass it was getting returned.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725571750
@nealrichardson @romainfrancois I've rebased and
- moved forward declarations to c++ headers, so it's no longer necessary to forward declare things in arrow_exports.h
- #include all type_fwd.h headers in arrow_types.h so all forward declarations are visible to `r/src/*`
- moved specializations of `r6_class_name` to arrow_types.h so they'll also be visible to `r/src/*`
- deleted arrow_exports.h; arrowExports.cpp now includes arrow_types.h
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724712441
I've added back an `as_sexp()` for `std::shared_ptr<...>` so that we can return the actual type instead of the catch-all R6, so for example:
```cpp
// [[arrow::export]]
std::shared_ptr<arrow::Field> Field__initialize(
const std::string& name, const std::shared_ptr<arrow::DataType>& field,
bool nullable = true) {
return arrow::field(name, field, nullable);
}
```
`as_sexp()` uses `cpp11::to_r6<>()` which in turns uses `r6_class_name<>()` to decide the R6 class name.
```cpp
template <typename T>
SEXP as_sexp(const std::shared_ptr<T>& ptr) {
return cpp11::to_r6<T>(ptr);
}
```
I have not been able to replace this template function with a template struct so far:
```cpp
template <typename T>
const char* r6_class_name(const std::shared_ptr<T>&);
```
this was giving errors about the type not being fully defined or something. I'm using a macro to help generating the many functions:
```cpp
#define DEFAULT_R6_CLASS_NAME(CLASS, NAME) \
namespace cpp11 { \
template <> \
const char* r6_class_name<CLASS>(const std::shared_ptr<CLASS>&) { \
return NAME; \
} \
}
```
so that for example:
```cpp
DEFAULT_R6_CLASS_NAME(parquet::ArrowReaderProperties, "ParquetArrowReaderProperties")
DEFAULT_R6_CLASS_NAME(parquet::ArrowWriterProperties, "ParquetArrowWriterProperties")
DEFAULT_R6_CLASS_NAME(parquet::WriterProperties, "ParquetWriterProperties")
DEFAULT_R6_CLASS_NAME(parquet::arrow::FileReader, "ParquetFileReader")
DEFAULT_R6_CLASS_NAME(parquet::WriterPropertiesBuilder, "ParquetWriterPropertiesBuilder")
DEFAULT_R6_CLASS_NAME(parquet::arrow::FileWriter, "ParquetFileWriter")
```
So that's still a little bit of housekeeping, and it's a bit brute force, but neither the R code (no more `shared_ptr()` or `..dispatch()`) nor the C++ side have to deal with conversion from a `std::shared_ptr<arrow::*>` to the right R6 class.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-698996362
> I'm not clear yet about the classes that are currently handled with `$..dispatch()` methods in R.
I can think of a few possibilities (as I'm sure you can):
1. List out all of the base classes that have the dispatch methods explicitly, just as you are with the class to R6 mapping, and do it directly (there are only 6 ..dispatch methods defined)
2. Detect whether the R6 object created has a ..dispatch method and if so, call it (though we'd need to be careful that this doesn't trigger recursively)
3. Presumably a function that has a return signature e.g. `std::shared_ptr<arrow::Array>` doesn't actually reveal that the returned object is `std::shared_ptr<arrow::StructArray>` or whatever subclass, but if it did, then ..dispatch goes away entirely and every class gets enumerated explicitly.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725059297
Sorry, my fault: the problem is I've moved the trait to arrow_exports.h so that arrowExports.cpp could access them because I was only thinking of the as_sexp usage. However compute.cpp can't always access them and so from_datum breaks. The solution is to ensure both the forward declarations and the traits are visible to all c++ code. The most straightforward way to accomplish that would be: inline arrow_exports.h into arrow_types.h (also entails moving the `r6_class_name` trait there)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz edited a comment on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-724995201
[Another sort of failure you might see:](https://github.com/apache/arrow/pull/8256/checks?check_run_id=1382178751#step:8:3439)
```
── 5. Failure: RecordBatchStreamReader / Writer (@test-record-batch-reader.R#31)
`writer` inherits from `RecordBatchWriter/ArrowObject/R6` not `RecordBatchStreamWriter`.
```
In R there are separate R6 classes `/RecordBatch(|Stream|File)Writer/` but in c++ there's only `RecordBatchWriter`. It does not provide any property which would tell you whether it's writing to a stream or a file so there's no way we can tell what the right R6 class would be. In this instance, it's a clue that we should probably delete the extra R6 classes since they're not doing anything except holding a `$create`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#issuecomment-725059297
Sorry; my fault: the problem is I've moved the trait to arrow_exports.h so that arrowExports.cpp could access them because I was only thinking of the as_sexp usage. However compute.cpp can't always access them and so from_datum breaks. The solution is to ensure both the forward declarations and the traits are visible to all c++ code. The most straightforward way to accomplish that would be: inline arrow_exports.h into arrow_types.h (also entails moving the `r6_class_name` trait there)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8256:
URL: https://github.com/apache/arrow/pull/8256#discussion_r514363942
##########
File path: r/src/dataset.cpp
##########
@@ -172,14 +220,13 @@ std::string dataset___FileFormat__type_name(
}
// [[arrow::export]]
-std::shared_ptr<ds::FileWriteOptions> dataset___FileFormat__DefaultWriteOptions(
- const std::shared_ptr<ds::FileFormat>& fmt) {
- return fmt->DefaultWriteOptions();
+R6 dataset___FileFormat__DefaultWriteOptions(const std::shared_ptr<ds::FileFormat>& fmt) {
+ return cpp11::r6(fmt->DefaultWriteOptions(), "FileWriteOptions");
}
// [[arrow::export]]
-std::shared_ptr<ds::ParquetFileFormat> dataset___ParquetFileFormat__Make(
- bool use_buffered_stream, int64_t buffer_size, cpp11::strings dict_columns) {
+R6 dataset___ParquetFileFormat__MakeRead(bool use_buffered_stream, int64_t buffer_size,
Review comment:
```suggestion
R6 dataset___ParquetFileFormat__Make(bool use_buffered_stream, int64_t buffer_size,
```
##########
File path: r/R/dataset-format.R
##########
@@ -98,9 +84,16 @@ as.character.FileFormat <- function(x, ...) {
ParquetFileFormat <- R6Class("ParquetFileFormat", inherit = FileFormat)
ParquetFileFormat$create <- function(use_buffered_stream = FALSE,
buffer_size = 8196,
- dict_columns = character(0)) {
- shared_ptr(ParquetFileFormat, dataset___ParquetFileFormat__Make(
- use_buffered_stream, buffer_size, dict_columns))
+ dict_columns = character(0),
+ writer_properties = NULL,
+ arrow_writer_properties = NULL) {
+ if (is.null(writer_properties) && is.null(arrow_writer_properties)) {
+ dataset___ParquetFileFormat__MakeRead(use_buffered_stream, buffer_size, dict_columns)
+ } else {
+ writer_properties = writer_properties %||% ParquetWriterProperties$create()
+ arrow_writer_properties = arrow_writer_properties %||% ParquetArrowWriterProperties$create()
+ dataset___ParquetFileFormat__MakeWrite(writer_properties, arrow_writer_properties)
+ }
Review comment:
We pulled the writer properties out of ParquetFileFormat
```suggestion
dict_columns = character(0)) {
dataset___ParquetFileFormat__Make(use_buffered_stream, buffer_size, dict_columns)
```
##########
File path: r/R/dataset.R
##########
@@ -145,29 +145,24 @@ open_dataset <- function(sources,
#' @seealso [open_dataset()] for a simple interface to creating a `Dataset`
Dataset <- R6Class("Dataset", inherit = ArrowObject,
public = list(
- ..dispatch = function() {
- type <- self$type
- if (type == "union") {
- shared_ptr(UnionDataset, self$pointer())
- } else if (type == "filesystem") {
- shared_ptr(FileSystemDataset, self$pointer())
- } else {
- self
- }
- },
# @description
# Start a new scan of the data
# @return A [ScannerBuilder]
- NewScan = function() unique_ptr(ScannerBuilder, dataset___Dataset__NewScan(self)),
- ToString = function() self$schema$ToString()
+ NewScan = function() dataset___Dataset__NewScan(self),
+ ToString = function() self$schema$ToString(),
+ write = function(path, filesystem = NULL, schema = self$schema, format, partitioning, ...) {
+ path_and_fs <- get_path_and_filesystem(path, filesystem)
+ dataset___Dataset__Write(self, schema, format, path_and_fs$fs, path_and_fs$path, partitioning)
+ invisible(self)
+ }
Review comment:
Another removal from the latest dataset writing patch
```suggestion
ToString = function() self$schema$ToString()
```
##########
File path: r/R/type.R
##########
@@ -37,51 +37,18 @@ DataType <- R6Class("DataType",
Equals = function(other, ...) {
inherits(other, "DataType") && DataType__Equals(self, other)
},
+ num_fields = function() {
+ DataType__num_fields(self)
+ },
num_children = function() {
- DataType__num_children(self)
+ DataType__num_fields(self)
},
children = function() {
- map(DataType__children_pointer(self), shared_ptr, class = Field)
+ # TODO: this is deprecated
+ DataType__fields(self)
},
Review comment:
If these are deprecated, we should just delete them. I think these are sufficiently buried (we haven't promoted any workflows that get into the methods of DataType objects, and these aren't documented) that they're safe to remove.
----------------------------------------------------------------
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:
users@infra.apache.org