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/10/05 12:22:57 UTC
[GitHub] [arrow] romainfrancois opened a new pull request #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
romainfrancois opened a new pull request #8341:
URL: https://github.com/apache/arrow/pull/8341
----------------------------------------------------------------
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 #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by 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
[GitHub] [arrow] nealrichardson commented on a change in pull request #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#discussion_r500365879
##########
File path: r/src/array_to_vector.cpp
##########
@@ -1068,8 +1080,9 @@ std::shared_ptr<Converter> Converter::Make(const std::shared_ptr<DataType>& type
return std::make_shared<arrow::r::Converter_Timestamp<int64_t>>(std::move(arrays));
case Type::INT64:
- // Prefer integer if it fits
- if (ArraysCanFitInteger(arrays)) {
+ // Prefer integer if it fits, unless option arrow.int64_auto_downcast is `false`
+ if (ArraysCanFitInteger(arrays) &&
+ GetBoolOption("arrow.int64_auto_downcast", true)) {
Review comment:
Your other comment said you were going with `arrow.int64_downcast` but that's not what is here. I'm ok either way though `arrow.int64_downcast` is more concise (and the "auto" behavior is really about what the default value is).
##########
File path: r/src/array_to_vector.cpp
##########
@@ -1068,8 +1080,9 @@ std::shared_ptr<Converter> Converter::Make(const std::shared_ptr<DataType>& type
return std::make_shared<arrow::r::Converter_Timestamp<int64_t>>(std::move(arrays));
case Type::INT64:
- // Prefer integer if it fits
- if (ArraysCanFitInteger(arrays)) {
+ // Prefer integer if it fits, unless option arrow.int64_auto_downcast is `false`
+ if (ArraysCanFitInteger(arrays) &&
Review comment:
Minor thought: should we switch the order of these checks? I have to imagine that GetBoolOption would be faster than doing bounds checking on the full array.
##########
File path: r/src/array_to_vector.cpp
##########
@@ -960,6 +960,18 @@ bool ArraysCanFitInteger(ArrayVector arrays) {
return all_can_fit;
}
+bool GetBoolOption(const std::string& name, bool default_) {
+ SEXP getOption = Rf_install("getOption");
+ cpp11::sexp call = Rf_lang2(getOption, Rf_mkString(name.c_str()));
+ cpp11::sexp res = Rf_eval(call, R_BaseEnv);
+ Rf_PrintValue(res);
Review comment:
Is this a debugging print statement you meant to remove?
```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 closed pull request #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #8341:
URL: https://github.com/apache/arrow/pull/8341
----------------------------------------------------------------
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 #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#issuecomment-703601865
https://issues.apache.org/jira/browse/ARROW-10093
----------------------------------------------------------------
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 #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#discussion_r500779786
##########
File path: r/src/array_to_vector.cpp
##########
@@ -960,6 +960,18 @@ bool ArraysCanFitInteger(ArrayVector arrays) {
return all_can_fit;
}
+bool GetBoolOption(const std::string& name, bool default_) {
+ SEXP getOption = Rf_install("getOption");
+ cpp11::sexp call = Rf_lang2(getOption, Rf_mkString(name.c_str()));
+ cpp11::sexp res = Rf_eval(call, R_BaseEnv);
+ Rf_PrintValue(res);
Review comment:
oops
----------------------------------------------------------------
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 #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#discussion_r500076684
##########
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:
using `arrow.int64_downcast`, happy to change
----------------------------------------------------------------
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 #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#issuecomment-703602038
Should that be a more general option perhaps ? Is this the right name ?
----------------------------------------------------------------
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 #8341: ARROW-10093: [R] Add ability to opt-out of int64 -> int demotion
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #8341:
URL: https://github.com/apache/arrow/pull/8341#discussion_r500074475
##########
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:
Went for `bool GetBoolOption(const std::string& name, bool default_)` so that we can specify what the default is when option is not set.
----------------------------------------------------------------
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