[GitHub] [arrow] nealrichardson commented on a change in pull request #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion

[GitBox <gi...@apache.org>]

nealrichardson commented on a change in pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#discussion_r499662830



##########
File path: r/src/array_to_vector.cpp
##########
@@ -960,6 +960,14 @@ bool ArraysCanFitInteger(ArrayVector arrays) {
   return all_can_fit;
 }
 
+bool option_arrow_disable_int64_auto_conversion() {

Review comment:
       How about we generalize this function since we're probably going to have more options:
   
   ```suggestion
   bool GetOption(std::string opt) {
   ```

##########
File path: r/tests/testthat/test-Array.R
##########
@@ -749,3 +749,17 @@ test_that("Array$ApproxEquals", {
   expect_true(a$ApproxEquals(b))
   expect_false(a$ApproxEquals(vec))
 })
+
+test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
+  op <- options(arrow_disable_int64_auto_conversion = TRUE); on.exit(options(op))

Review comment:
       Re: naming, we currently have options "arrow.use_threads" (mostly private but some may need to set it given the multithreading bugs we've observed) and "arrow.dev.repo" (not advertised). I don't feel strongly about the naming convention as long as we're consistent and predictable (which, naturally, `names(options())` is not). 
   
   Searching around for other conventions, I found `package.option_name` (cf. https://github.com/tidyverse/dplyr/pull/5548) like we did with `arrow.use_threads`, so maybe let's standardize on that?




----------------------------------------------------------------
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