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 2021/12/09 17:21:12 UTC
[GitHub] [arrow] paleolimbot opened a new pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
paleolimbot opened a new pull request #11919:
URL: https://github.com/apache/arrow/pull/11919
This PR limits the use of external pointers whose value is casted to double. It only uses the double-casted pointer when passing a pointer to Python because this is the only way I can get this to work (perhaps there is a better way that will result from this review or that could be implemented in the future). The PR changes the implementation of the `Pointer<>` wrapper to:
- Do the right thing and use R's externalptr type internally (and accept pointers defined in this way).
- Accept pointers defined as `void*` casted to `uintptr_t` casted to `double` (for backward compatibility)
- Accept pointers defined as `integer64` (because it's what @amol- thought to do at first and somebody else might, too)
- Accept pointers defined as `raw(<pointer size>)` (because it's a way that pointers could get passed to Python without changing reticulate)
- Accept pointers defined as a character representation (as parsed by `strtoll()`) (as suggested by @pitrou)
I imagine that we don't have to use one or more of these, but thought it best to implement and test them all and use the review to eliminate any options that will never get used.
Things I'm not sure about:
- My implementations of parsing and exporting pointers (new to this!)
- The non-unwind-protected-ness of the `Pointer<>` constructor...it's written such that it's unlikely to longjmp but could possibly use `cpp11::safe[]()` to make this impossible. This doesn't seem to be used elsewhere in the Arrow R package so I didn't implement it here.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772402389
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
The pointer cast approach leads to undefined behavior (see "Type aliasing" in https://en.cppreference.com/w/cpp/language/reinterpret_cast), so `memcpy` is the right way to do it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
amol- commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772272224
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
I think we can avoid the `memcpy` doing something like
```
ptr_ = *reinterpret_cast<T**>(&REAL(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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772279116
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
The `memcpy` is the standard method of doing this in a well-defined way. It is optimized away by the compiler.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-990070903
> It only uses the double-casted pointer when passing a pointer to Python because this is the only way I can get this to work (perhaps there is a better way that will result from this review or that could be implemented in the future).
I think we can do that in a future PR indeed.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772353071
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
I don't have an opinion on which is better, other than that it probably has to work on 32-bit Windows where pointers probably aren't 8 bytes.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766111948
##########
File path: r/src/arrowExports.cpp
##########
@@ -5,55 +5,55 @@
#include "./arrow_types.h"
// altrep.cpp
-#if defined(ARROW_R_WITH_ARROW)
-void test_SET_STRING_ELT(SEXP s);
+ #if defined(ARROW_R_WITH_ARROW)
+ void test_SET_STRING_ELT(SEXP s);
Review comment:
Thank you for tracking this down and sorry for the indirection...I re-rendered with glue 1.5.0 and opened a JIRA about it (ARROW-15049).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772353071
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
I don't have an opinion on which is better, other than that it has to work on 32-bit Windows where pointers probably aren't 8 bytes.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766656781
##########
File path: r/src/py-to-r.cpp
##########
@@ -21,6 +21,38 @@
#include <arrow/c/bridge.h>
+// [[arrow::export]]
+double external_pointer_addr_double(SEXP external_pointer) {
+ // potentially lossy conversion to double needed for the current
+ // implementation of import/export to Python
+ return reinterpret_cast<uintptr_t>(R_ExternalPtrAddr(external_pointer));
Review comment:
I think this one needs to stay like this because it's the exact code that was previously used to convert a `Pointer<>` to `SEXP` (and is what the Python library relies on to recreate the pointer).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-994936704
Yes, it's ready for another round of review!
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766018570
##########
File path: r/src/py-to-r.cpp
##########
@@ -21,6 +21,41 @@
#include <arrow/c/bridge.h>
+// [[arrow::export]]
+double external_pointer_addr_double(SEXP external_pointer) {
+ // potentially lossy conversion to double needed for the current
+ // implementation of import/export to Python
+ return reinterpret_cast<uintptr_t>(R_ExternalPtrAddr(external_pointer));
+}
+
+// [[arrow::export]]
+std::string external_pointer_addr_character(SEXP external_pointer) {
+ void* ptr_void = R_ExternalPtrAddr(external_pointer);
+ uint64_t ptr_int64 = reinterpret_cast<uintptr_t>(ptr_void);
+ char ptr_chars[128];
+ memset(ptr_chars, 0, 128);
+ int nchar = sprintf(ptr_chars, "%llu", ptr_int64);
Review comment:
You can use `std::to_string`: https://en.cppreference.com/w/cpp/string/basic_string/to_string
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-998144871
Benchmark runs are scheduled for baseline = 04641a38ee5da0fa1d745fd29fd4a206719338cd and contender = aade058c32d19c5375284a2560d8b8e497556c7f. aade058c32d19c5375284a2560d8b8e497556c7f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f3bd225175d4420682930f4e4dea2508...8beade6b600748eeb1941da03e4fff5e/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ab57c6fc967444e599dbe7014b58e17f...f25e63b46699485ab94207f6b8218998/)
[Finished :arrow_down:0.09% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/96d2df3131364541be794edff1c072fe...6d476c5073274d0a89a1a7cc112177e4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-990057651
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766050050
##########
File path: r/src/arrowExports.cpp
##########
@@ -5,55 +5,55 @@
#include "./arrow_types.h"
// altrep.cpp
-#if defined(ARROW_R_WITH_ARROW)
-void test_SET_STRING_ELT(SEXP s);
+ #if defined(ARROW_R_WITH_ARROW)
+ void test_SET_STRING_ELT(SEXP s);
Review comment:
I'll look into this...I think a cpp11 update caused this change (the file is auto-generated) but I'll see if I can minimize the diff for this PR.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766100971
##########
File path: r/src/arrowExports.cpp
##########
@@ -5,55 +5,55 @@
#include "./arrow_types.h"
// altrep.cpp
-#if defined(ARROW_R_WITH_ARROW)
-void test_SET_STRING_ELT(SEXP s);
+ #if defined(ARROW_R_WITH_ARROW)
+ void test_SET_STRING_ELT(SEXP s);
Review comment:
Maybe `glue` changed how it deals with leading whitespace? https://github.com/apache/arrow/blob/master/r/data-raw/codegen.R#L126-L135
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766146825
##########
File path: r/tests/testthat/test-python.R
##########
@@ -143,3 +143,70 @@ test_that("RecordBatchReader from python", {
expect_r6_class(rt_table, "Table")
expect_identical(as.data.frame(rt_table), example_data)
})
+
+test_that("Pointer wrapper accepts external pointers", {
Review comment:
I agree! Should I do that (and rename the corresponding .cpp file) in this PR?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766015524
##########
File path: r/src/arrowExports.cpp
##########
@@ -5,55 +5,55 @@
#include "./arrow_types.h"
// altrep.cpp
-#if defined(ARROW_R_WITH_ARROW)
-void test_SET_STRING_ELT(SEXP s);
+ #if defined(ARROW_R_WITH_ARROW)
+ void test_SET_STRING_ELT(SEXP s);
Review comment:
Why are there so many changes in this file? Did your editor apply automatic changes? If so, it would be nice to disable them.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766141468
##########
File path: r/tests/testthat/test-python.R
##########
@@ -143,3 +143,70 @@ test_that("RecordBatchReader from python", {
expect_r6_class(rt_table, "Table")
expect_identical(as.data.frame(rt_table), example_data)
})
+
+test_that("Pointer wrapper accepts external pointers", {
Review comment:
It's totally fine to keep these here, they certainly are mostly tested with Python, but I wonder if these should go in a file dedicated to c-stream interactions (especially now that we have DuckDB using some of the same infrastructure
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed an character representation of the pointer address
Review comment:
Super minor
```suggestion
// User passed a character representation of the pointer address
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-998144871
Benchmark runs are scheduled for baseline = 04641a38ee5da0fa1d745fd29fd4a206719338cd and contender = aade058c32d19c5375284a2560d8b8e497556c7f. aade058c32d19c5375284a2560d8b8e497556c7f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f3bd225175d4420682930f4e4dea2508...8beade6b600748eeb1941da03e4fff5e/)
[Failed :arrow_down:0.45% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ab57c6fc967444e599dbe7014b58e17f...f25e63b46699485ab94207f6b8218998/)
[Finished :arrow_down:0.09% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/96d2df3131364541be794edff1c072fe...6d476c5073274d0a89a1a7cc112177e4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
amol- commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772280504
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
Oh, that's interesting, based on which rules the compiler decides that the `memcpy` can be avoided?
I mean, `memcpy` for fixed size is usually optimized to assignments, but in the end that's still doing some extra work compared to just a cast that translates to no cpu instructions
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] amol- commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
amol- commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772280504
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
Oh, that's interesting, based on which rules the compiler decides that the `memcpy` can be avoided?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-998144871
Benchmark runs are scheduled for baseline = 04641a38ee5da0fa1d745fd29fd4a206719338cd and contender = aade058c32d19c5375284a2560d8b8e497556c7f. aade058c32d19c5375284a2560d8b8e497556c7f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f3bd225175d4420682930f4e4dea2508...8beade6b600748eeb1941da03e4fff5e/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ab57c6fc967444e599dbe7014b58e17f...f25e63b46699485ab94207f6b8218998/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/96d2df3131364541be794edff1c072fe...6d476c5073274d0a89a1a7cc112177e4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-998144871
Benchmark runs are scheduled for baseline = 04641a38ee5da0fa1d745fd29fd4a206719338cd and contender = aade058c32d19c5375284a2560d8b8e497556c7f. aade058c32d19c5375284a2560d8b8e497556c7f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/f3bd225175d4420682930f4e4dea2508...8beade6b600748eeb1941da03e4fff5e/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ab57c6fc967444e599dbe7014b58e17f...f25e63b46699485ab94207f6b8218998/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/96d2df3131364541be794edff1c072fe...6d476c5073274d0a89a1a7cc112177e4/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766019940
##########
File path: r/tests/testthat/test-python.R
##########
@@ -143,3 +143,70 @@ test_that("RecordBatchReader from python", {
expect_r6_class(rt_table, "Table")
expect_identical(as.data.frame(rt_table), example_data)
})
+
+test_that("Pointer wrapper accepts external pointers", {
+ ptr <- allocate_arrow_schema()
+ exportable <- int32()
+ exportable$export_to_c(ptr)
+
+ # make sure exportable is released and deleted
+ expect_equal(DataType$import_from_c(ptr), int32())
+ delete_arrow_schema(ptr)
+})
+
+test_that("Pointer wrapper accepts double-casted pointers", {
+ ptr <- allocate_arrow_schema()
+ exportable <- int32()
+ exportable$export_to_c(external_pointer_addr_double(ptr))
+
+ # make sure exportable is released and deleted
+ expect_equal(DataType$import_from_c(ptr), int32())
Review comment:
Should `import_from_c` also take `external_pointer_addr_double(ptr)`?
(same question below)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou edited a comment on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou edited a comment on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-990070903
> It only uses the double-casted pointer when passing a pointer to Python because this is the only way I can get this to work (perhaps there is a better way that will result from this review or that could be implemented in the future).
I think we can do that in a future PR indeed: we should be able to accept the Python "capsule object" that's produced by `r_to_py`.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766100486
##########
File path: r/src/arrowExports.cpp
##########
@@ -5,55 +5,55 @@
#include "./arrow_types.h"
// altrep.cpp
-#if defined(ARROW_R_WITH_ARROW)
-void test_SET_STRING_ELT(SEXP s);
+ #if defined(ARROW_R_WITH_ARROW)
+ void test_SET_STRING_ELT(SEXP s);
Review comment:
This file is all generated by https://github.com/apache/arrow/blob/master/r/data-raw/codegen.R, not cpp11. Neither have changed recently (cpp11 is vendored currently)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766524355
##########
File path: r/src/py-to-r.cpp
##########
@@ -21,6 +21,38 @@
#include <arrow/c/bridge.h>
+// [[arrow::export]]
+double external_pointer_addr_double(SEXP external_pointer) {
+ // potentially lossy conversion to double needed for the current
+ // implementation of import/export to Python
+ return reinterpret_cast<uintptr_t>(R_ExternalPtrAddr(external_pointer));
Review comment:
Can this be something like ?
```c
double out;
void* ptr = R_ExternalPtrAddr(external_pointer);
memcpy(&out, R_ExternalPtrAddr(external_pointer), sizeof(double));
return out;
```
##########
File path: r/src/py-to-r.cpp
##########
@@ -21,6 +21,38 @@
#include <arrow/c/bridge.h>
+// [[arrow::export]]
+double external_pointer_addr_double(SEXP external_pointer) {
+ // potentially lossy conversion to double needed for the current
+ // implementation of import/export to Python
+ return reinterpret_cast<uintptr_t>(R_ExternalPtrAddr(external_pointer));
Review comment:
It seems to be what is done in `external_pointer_addr_integer64()` below. Maybe if this returned a `cpp11::doubles` ?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r767831587
##########
File path: r/tests/testthat/test-python.R
##########
@@ -143,3 +143,70 @@ test_that("RecordBatchReader from python", {
expect_r6_class(rt_table, "Table")
expect_identical(as.data.frame(rt_table), example_data)
})
+
+test_that("Pointer wrapper accepts external pointers", {
Review comment:
If it's not too much trouble, go for it
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#issuecomment-994932365
Is this ready for another review round @paleolimbot ?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jonkeane closed pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #11919:
URL: https://github.com/apache/arrow/pull/11919
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r772294483
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed a character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoull(input_chars, &endptr, 0);
+ if (endptr != (input_chars + strlen(input_chars))) {
+ cpp11::stop("Can't parse '%s' as a 64-bit integer address", input_chars);
+ }
- inline operator SEXP() const {
- return Rf_ScalarReal(static_cast<double>(reinterpret_cast<uintptr_t>(ptr_)));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
+ } else if (Rf_inherits(x, "integer64") && Rf_length(x) == 1) {
+ // User passed an integer64(1) of the pointer address
+ // an integer64 is a REALSXP under the hood, with the bytes
+ // of each double reinterpreted as an int64.
+ uint64_t ptr_value;
+ memcpy(&ptr_value, REAL(x), sizeof(uint64_t));
+ ptr_ = reinterpret_cast<T*>(static_cast<uintptr_t>(ptr_value));
Review comment:
I don't know whether the assignment would be optimized away, but assignments between CPU registers are practically costless.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766017291
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed an character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoll(input_chars, &endptr, 0);
Review comment:
Perhaps use `strotoull`, since you're converting to unsigned?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766791379
##########
File path: r/src/py-to-r.cpp
##########
@@ -21,6 +21,38 @@
#include <arrow/c/bridge.h>
+// [[arrow::export]]
+double external_pointer_addr_double(SEXP external_pointer) {
+ // potentially lossy conversion to double needed for the current
+ // implementation of import/export to Python
+ return reinterpret_cast<uintptr_t>(R_ExternalPtrAddr(external_pointer));
Review comment:
For sure! The motivation of this PR is to provide all the machinery on the R side that we need to avoid doing it (with a PR on the Python side to follow).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paleolimbot commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766058487
##########
File path: r/tests/testthat/test-python.R
##########
@@ -143,3 +143,70 @@ test_that("RecordBatchReader from python", {
expect_r6_class(rt_table, "Table")
expect_identical(as.data.frame(rt_table), example_data)
})
+
+test_that("Pointer wrapper accepts external pointers", {
+ ptr <- allocate_arrow_schema()
+ exportable <- int32()
+ exportable$export_to_c(ptr)
+
+ # make sure exportable is released and deleted
+ expect_equal(DataType$import_from_c(ptr), int32())
+ delete_arrow_schema(ptr)
+})
+
+test_that("Pointer wrapper accepts double-casted pointers", {
+ ptr <- allocate_arrow_schema()
+ exportable <- int32()
+ exportable$export_to_c(external_pointer_addr_double(ptr))
+
+ # make sure exportable is released and deleted
+ expect_equal(DataType$import_from_c(ptr), int32())
Review comment:
They do! It's the `Pointer<>` wrapper that's automatically invoked for all import/export methods (but I added the test for the import method for DataType also because it was easy). I could also test more than just `DataType`'s import/export but it's just a proxy for the `Pointer<>` here.
##########
File path: r/src/arrow_cpp11.h
##########
@@ -57,13 +57,45 @@ namespace r {
template <typename T>
struct Pointer {
Pointer() : ptr_(new T()) {}
- explicit Pointer(SEXP x)
- : ptr_(reinterpret_cast<T*>(static_cast<uintptr_t>(REAL(x)[0]))) {}
+ explicit Pointer(SEXP x) {
+ if (TYPEOF(x) == EXTPTRSXP) {
+ ptr_ = (T*)R_ExternalPtrAddr(x);
+ } else if (TYPEOF(x) == STRSXP && Rf_length(x) == 1) {
+ // User passed an character representation of the pointer address
+ SEXP char0 = STRING_ELT(x, 0);
+ if (char0 == NA_STRING) {
+ cpp11::stop("Can't convert NA_character_ to pointer");
+ }
+
+ const char* input_chars = CHAR(char0);
+ char* endptr;
+ uint64_t ptr_value = strtoll(input_chars, &endptr, 0);
Review comment:
Done!
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] romainfrancois commented on a change in pull request #11919: ARROW-14804: [R] import_from_c() / export_to_c() methods should accept external pointers
Posted by GitBox <gi...@apache.org>.
romainfrancois commented on a change in pull request #11919:
URL: https://github.com/apache/arrow/pull/11919#discussion_r766734547
##########
File path: r/src/py-to-r.cpp
##########
@@ -21,6 +21,38 @@
#include <arrow/c/bridge.h>
+// [[arrow::export]]
+double external_pointer_addr_double(SEXP external_pointer) {
+ // potentially lossy conversion to double needed for the current
+ // implementation of import/export to Python
+ return reinterpret_cast<uintptr_t>(R_ExternalPtrAddr(external_pointer));
Review comment:
I see. It sort of feels wrong though.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org