thisisnic commented on code in PR #51288:
URL: https://github.com/apache/arrow/pull/51288#discussion_r3981378166
##########
r/src/altrep.cpp:
##########
@@ -857,29 +857,111 @@ struct AltrepVectorString : public
AltrepVectorBase<AltrepVectorString<Type>> {
std::string_view view_;
};
- // Get a single string as a CHARSXP SEXP
- static SEXP Elt(SEXP alt, R_xlen_t i) {
- if (Base::IsMaterialized(alt)) {
- return STRING_ELT(Representation(alt), i);
+ // Strings created by Elt() must stay reachable for as long as the altrep
+ // vector does: base R may hold the CHARSXP returned by STRING_ELT() across
+ // a later allocation, and a CHARSXP that nothing references is collected
+ // (GH-51198). Until the vector is materialized they are kept in fixed-size
+ // STRSXP blocks hanging off a VECSXP stored in the protected slot of the
+ // data1 external pointer. Blocks are allocated on first touch, so sparse
+ // access costs one block rather than a full-length vector. Within a block,
+ // R_BlankString marks a slot that has not been converted yet: the empty
+ // string is a permanent singleton, so converting it again is harmless.
+ static constexpr R_xlen_t kCacheBlockShift = 10;
+ static constexpr R_xlen_t kCacheBlockSize = R_xlen_t(1) << kCacheBlockShift;
+ static constexpr R_xlen_t kCacheBlockMask = kCacheBlockSize - 1;
+
+ // The cache block holding element i, allocating it (and the list of
+ // blocks) on first use. Only valid while the vector is not materialized.
+ static SEXP CacheBlock(SEXP alt, R_xlen_t i) {
+ SEXP data1 = R_altrep_data1(alt);
+ R_xlen_t length = GetChunkedArray(alt)->length();
+
+ SEXP blocks = R_ExternalPtrProtected(data1);
+ if (Rf_isNull(blocks)) {
+ R_xlen_t n_blocks = (length + kCacheBlockMask) >> kCacheBlockShift;
+ blocks = PROTECT(Rf_allocVector(VECSXP, n_blocks));
+ R_SetExternalPtrProtected(data1, blocks);
+ UNPROTECT(1);
}
+ R_xlen_t b = i >> kCacheBlockShift;
+ SEXP block = VECTOR_ELT(blocks, b);
+ if (Rf_isNull(block)) {
+ R_xlen_t block_length = std::min(kCacheBlockSize, length - (b <<
kCacheBlockShift));
+ block = PROTECT(Rf_allocVector(STRSXP, block_length));
+ SET_VECTOR_ELT(blocks, b, block);
+ UNPROTECT(1);
+ }
Review Comment:
I don't think we need it; we use `std::find` just fine in 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
