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/11/10 16:51:24 UTC

[GitHub] [arrow] nealrichardson commented on a change in pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function

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