From 3b461625505c621557c28226d5260b1d0c55e4fb Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Mon, 26 Oct 2020 15:08:15 -0700 Subject: [PATCH 1/2] Fix performance regression from cpp11::r_string --- r/src/array_to_vector.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/r/src/array_to_vector.cpp b/r/src/array_to_vector.cpp index 51f0e3308ef..7c4f6a33773 100644 --- a/r/src/array_to_vector.cpp +++ b/r/src/array_to_vector.cpp @@ -294,7 +294,7 @@ struct Converter_String : public Converter { array->offset(), n); for (int i = 0; i < n; i++, null_reader.Next()) { if (null_reader.IsSet()) { - SET_STRING_ELT(data, start + i, cpp11::r_string(string_array->GetString(i))); + SET_STRING_ELT(data, start + i, r_string_from_view(string_array->GetView(i))); } else { SET_STRING_ELT(data, start + i, NA_STRING); } @@ -302,7 +302,7 @@ struct Converter_String : public Converter { } else { for (int i = 0; i < n; i++) { - SET_STRING_ELT(data, start + i, cpp11::r_string(string_array->GetString(i))); + SET_STRING_ELT(data, start + i, r_string_from_view(string_array->GetView(i))); } } @@ -310,6 +310,9 @@ struct Converter_String : public Converter { } bool Parallel() const { return false; } + inline SEXP r_string_from_view(const arrow::util::string_view& view) const { + return Rf_mkCharLenCE(view.data(), view.size(), CE_UTF8); + } }; class Converter_Boolean : public Converter { From d6037ccce5f7081db8d164618ff40a27c1f49b85 Mon Sep 17 00:00:00 2001 From: Neal Richardson Date: Tue, 27 Oct 2020 08:11:03 -0700 Subject: [PATCH 2/2] Apply @bkietz's suggestions --- r/src/array_to_vector.cpp | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/r/src/array_to_vector.cpp b/r/src/array_to_vector.cpp index 7c4f6a33773..2b33e056b4a 100644 --- a/r/src/array_to_vector.cpp +++ b/r/src/array_to_vector.cpp @@ -292,25 +292,30 @@ struct Converter_String : public Converter { // need to watch for nulls arrow::internal::BitmapReader null_reader(array->null_bitmap_data(), array->offset(), n); - for (int i = 0; i < n; i++, null_reader.Next()) { - if (null_reader.IsSet()) { - SET_STRING_ELT(data, start + i, r_string_from_view(string_array->GetView(i))); - } else { - SET_STRING_ELT(data, start + i, NA_STRING); + cpp11::unwind_protect([&] { + for (int i = 0; i < n; i++, null_reader.Next()) { + if (null_reader.IsSet()) { + SET_STRING_ELT(data, start + i, r_string_from_view(string_array->GetView(i))); + } else { + SET_STRING_ELT(data, start + i, NA_STRING); + } } - } - + }); } else { - for (int i = 0; i < n; i++) { - SET_STRING_ELT(data, start + i, r_string_from_view(string_array->GetView(i))); - } + cpp11::unwind_protect([&] { + for (int i = 0; i < n; i++) { + SET_STRING_ELT(data, start + i, r_string_from_view(string_array->GetView(i))); + } + }); } return Status::OK(); } bool Parallel() const { return false; } - inline SEXP r_string_from_view(const arrow::util::string_view& view) const { + + private: + static SEXP r_string_from_view(const arrow::util::string_view& view) { return Rf_mkCharLenCE(view.data(), view.size(), CE_UTF8); } };