From 908d1c851959ad6022e05c096776f53fa5a5a867 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Mon, 5 Oct 2020 14:17:36 +0200 Subject: [PATCH 1/5] react to option "arrow_disable_int64_auto_conversion" --- r/src/array_to_vector.cpp | 10 +++++++++- r/tests/testthat/test-Array.R | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/r/src/array_to_vector.cpp b/r/src/array_to_vector.cpp index f790ab87da6..e8c1d25db62 100644 --- a/r/src/array_to_vector.cpp +++ b/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() { + SEXP getOption = Rf_install("getOption"); + cpp11::sexp call = + Rf_lang2(getOption, Rf_mkString("arrow_disable_int64_auto_conversion")); + cpp11::sexp res = Rf_eval(call, R_BaseEnv); + return TYPEOF(res) == LGLSXP && LOGICAL(res)[0] == TRUE; +} + std::shared_ptr Converter::Make(const std::shared_ptr& type, ArrayVector arrays) { if (arrays.empty()) { @@ -1069,7 +1077,7 @@ std::shared_ptr Converter::Make(const std::shared_ptr& type case Type::INT64: // Prefer integer if it fits - if (ArraysCanFitInteger(arrays)) { + if (ArraysCanFitInteger(arrays) && !option_arrow_disable_int64_auto_conversion()) { return std::make_shared>( std::move(arrays)); } else { diff --git a/r/tests/testthat/test-Array.R b/r/tests/testthat/test-Array.R index d708cb9676f..7ed95475577 100644 --- a/r/tests/testthat/test-Array.R +++ b/r/tests/testthat/test-Array.R @@ -749,3 +749,9 @@ 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)) + expect_true(inherits(Array$create(1:10, int64())$as_vector(), "integer64")) +}) + From 1532048c1d6a047f7a1fc2139583458366359182 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Mon, 5 Oct 2020 14:27:15 +0200 Subject: [PATCH 2/5] Also test through record batch and Table --- r/tests/testthat/test-Array.R | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/r/tests/testthat/test-Array.R b/r/tests/testthat/test-Array.R index 7ed95475577..e2437d592e4 100644 --- a/r/tests/testthat/test-Array.R +++ b/r/tests/testthat/test-Array.R @@ -752,6 +752,14 @@ test_that("Array$ApproxEquals", { test_that("auto int64 conversion to int can be disabled (ARROW-10093)", { op <- options(arrow_disable_int64_auto_conversion = TRUE); on.exit(options(op)) - expect_true(inherits(Array$create(1:10, int64())$as_vector(), "integer64")) + + a <- Array$create(1:10, int64()) + expect_true(inherits(a$as_vector(), "integer64")) + + batch <- RecordBatch$create(x = a) + expect_true(inherits(as.data.frame(batch)$x, "integer64")) + + tab <- Table$create(x = a) + expect_true(inherits(as.data.frame(batch)$x, "integer64")) }) From 7c5f08aee8795533c566d46433eac3d4ae152623 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Tue, 6 Oct 2020 09:53:04 +0200 Subject: [PATCH 3/5] generatlize GetOption, and name "arrow.int64_downcast" --- r/src/array_to_vector.cpp | 17 +++++++++++------ r/src/arrow_cpp11.h | 2 ++ r/tests/testthat/test-Array.R | 2 +- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/r/src/array_to_vector.cpp b/r/src/array_to_vector.cpp index e8c1d25db62..0530b1121b3 100644 --- a/r/src/array_to_vector.cpp +++ b/r/src/array_to_vector.cpp @@ -960,12 +960,16 @@ bool ArraysCanFitInteger(ArrayVector arrays) { return all_can_fit; } -bool option_arrow_disable_int64_auto_conversion() { +bool GetBoolOption(const std::string& name, bool default_) { SEXP getOption = Rf_install("getOption"); - cpp11::sexp call = - Rf_lang2(getOption, Rf_mkString("arrow_disable_int64_auto_conversion")); + cpp11::sexp call = Rf_lang2(getOption, Rf_mkString(name.c_str())); cpp11::sexp res = Rf_eval(call, R_BaseEnv); - return TYPEOF(res) == LGLSXP && LOGICAL(res)[0] == TRUE; + Rf_PrintValue(res); + if (TYPEOF(res) == LGLSXP) { + return LOGICAL(res)[0] == TRUE; + } else { + return default_; + } } std::shared_ptr Converter::Make(const std::shared_ptr& type, @@ -1076,8 +1080,9 @@ std::shared_ptr Converter::Make(const std::shared_ptr& type return std::make_shared>(std::move(arrays)); case Type::INT64: - // Prefer integer if it fits - if (ArraysCanFitInteger(arrays) && !option_arrow_disable_int64_auto_conversion()) { + // Prefer integer if it fits, unless option arrow.int64_auto_downcast is `false` + if (ArraysCanFitInteger(arrays) && + GetBoolOption("arrow.int64_auto_downcast", true)) { return std::make_shared>( std::move(arrays)); } else { diff --git a/r/src/arrow_cpp11.h b/r/src/arrow_cpp11.h index 1b69468b3bf..859b0491cd0 100644 --- a/r/src/arrow_cpp11.h +++ b/r/src/arrow_cpp11.h @@ -292,6 +292,8 @@ std::vector from_r_list(cpp11::list args) { return vec; } +bool GetBoolOption(const std::string& name, bool default_); + } // namespace r } // namespace arrow diff --git a/r/tests/testthat/test-Array.R b/r/tests/testthat/test-Array.R index e2437d592e4..92d01a9ec85 100644 --- a/r/tests/testthat/test-Array.R +++ b/r/tests/testthat/test-Array.R @@ -751,7 +751,7 @@ test_that("Array$ApproxEquals", { }) test_that("auto int64 conversion to int can be disabled (ARROW-10093)", { - op <- options(arrow_disable_int64_auto_conversion = TRUE); on.exit(options(op)) + op <- options(arrow.int64_auto_downcast = FALSE); on.exit(options(op)) a <- Array$create(1:10, int64()) expect_true(inherits(a$as_vector(), "integer64")) From cbe87e1aeb0fe39c562b265ba955966b365002e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Wed, 7 Oct 2020 08:59:53 +0200 Subject: [PATCH 4/5] Update r/src/array_to_vector.cpp Co-authored-by: Neal Richardson --- r/src/array_to_vector.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/r/src/array_to_vector.cpp b/r/src/array_to_vector.cpp index 0530b1121b3..73e2e7bebae 100644 --- a/r/src/array_to_vector.cpp +++ b/r/src/array_to_vector.cpp @@ -964,7 +964,6 @@ 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); if (TYPEOF(res) == LGLSXP) { return LOGICAL(res)[0] == TRUE; } else { From c1b40ecd90eb3a53bff013e64e781ba52c6f36e9 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Wed, 7 Oct 2020 09:03:40 +0200 Subject: [PATCH 5/5] using arrow.int64_downcast --- r/src/array_to_vector.cpp | 5 ++--- r/tests/testthat/test-Array.R | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/r/src/array_to_vector.cpp b/r/src/array_to_vector.cpp index 73e2e7bebae..f0a7af8f29c 100644 --- a/r/src/array_to_vector.cpp +++ b/r/src/array_to_vector.cpp @@ -1079,9 +1079,8 @@ std::shared_ptr Converter::Make(const std::shared_ptr& type return std::make_shared>(std::move(arrays)); case Type::INT64: - // Prefer integer if it fits, unless option arrow.int64_auto_downcast is `false` - if (ArraysCanFitInteger(arrays) && - GetBoolOption("arrow.int64_auto_downcast", true)) { + // Prefer integer if it fits, unless option arrow.int64_downcast is `false` + if (GetBoolOption("arrow.int64_downcast", true) && ArraysCanFitInteger(arrays)) { return std::make_shared>( std::move(arrays)); } else { diff --git a/r/tests/testthat/test-Array.R b/r/tests/testthat/test-Array.R index 92d01a9ec85..59c6dd9866a 100644 --- a/r/tests/testthat/test-Array.R +++ b/r/tests/testthat/test-Array.R @@ -751,7 +751,7 @@ test_that("Array$ApproxEquals", { }) test_that("auto int64 conversion to int can be disabled (ARROW-10093)", { - op <- options(arrow.int64_auto_downcast = FALSE); on.exit(options(op)) + op <- options(arrow.int64_downcast = FALSE); on.exit(options(op)) a <- Array$create(1:10, int64()) expect_true(inherits(a$as_vector(), "integer64"))