[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function

2020-11-10 Thread GitBox 


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:
us...@infra.apache.org

[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function

2020-11-10 Thread GitBox 


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:
us...@infra.apache.org

[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function

2020-11-10 Thread GitBox 


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:
us...@infra.apache.org

[GitHub] [arrow] bkietz edited a comment on pull request #8256: ARROW-9001: [R] Box outputs as correct type in call_function

2020-11-10 Thread GitBox 


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 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:
us...@infra.apache.org