jonkeane commented on code in PR #45951:
URL: https://github.com/apache/arrow/pull/45951#discussion_r2019675135
##########
r/src/altrep.cpp:
##########
@@ -1267,24 +1304,27 @@ sexp test_arrow_altrep_copy_by_dataptr(sexp x) {
if (TYPEOF(x) == INTSXP) {
cpp11::writable::integers out(Rf_xlength(x));
- int* ptr = reinterpret_cast<int*>(DATAPTR(x));
+ int* ptr = reinterpret_cast<int*>(INTEGER(x));
for (R_xlen_t i = 0; i < n; i++) {
out[i] = ptr[i];
}
return out;
} else if (TYPEOF(x) == REALSXP) {
cpp11::writable::doubles out(Rf_xlength(x));
- double* ptr = reinterpret_cast<double*>(DATAPTR(x));
+ double* ptr = reinterpret_cast<double*>(REAL(x));
for (R_xlen_t i = 0; i < n; i++) {
out[i] = ptr[i];
}
return out;
} else if (TYPEOF(x) == STRSXP) {
cpp11::writable::strings out(Rf_xlength(x));
- SEXP* ptr = reinterpret_cast<SEXP*>(DATAPTR(x));
- for (R_xlen_t i = 0; i < n; i++) {
- out[i] = ptr[i];
+ R_xlen_t len = Rf_xlength(x);
+
+ for (R_xlen_t i = 0; i < len; i++) {
+ SEXP str_elt = reinterpret_cast<SEXP>(STRING_ELT(x, i));
+ out[i] = str_elt;
Review Comment:
Something about this isn't quite right, because the test here (and other
assertions that strings are materialized) fail:
https://github.com/apache/arrow/blob/3c63f37d4211e7a8d42c8388a12888e63fe377e3/r/tests/testthat/test-altrep.R#L249
But I haven't yet figured out if this is a real problem with this code
change, or maybe it's an assumption in the tests that no longer holds?
@nealrichardson @paleolimbot y'all might have thoughts ?
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
