thisisnic commented on code in PR #49712:
URL: https://github.com/apache/arrow/pull/49712#discussion_r3804421841
##########
r/src/r_to_arrow.cpp:
##########
@@ -862,54 +862,97 @@ class RPrimitiveConverter<T, enable_if_t<std::is_same<T,
FixedSizeBinaryType>::v
};
template <typename T>
-class RPrimitiveConverter<T, enable_if_string_like<T>>
+class RPrimitiveConverter<
+ T, enable_if_t<is_string_like_type<T>::value ||
is_string_view_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
public:
- using OffsetType = typename T::offset_type;
-
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
RVectorType rtype = GetVectorType(x);
if (rtype != STRING) {
return Status::Invalid("Expecting a character vector");
}
- return UnsafeAppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
+ return AppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
}
void DelayedExtend(SEXP values, int64_t size, RTasks& tasks) override {
auto task = [this, values, size]() { return this->Extend(values, size); };
- // TODO: refine this., e.g. extract setup from Extend()
tasks.Append(false, std::move(task));
}
private:
- Status UnsafeAppendUtf8Strings(const cpp11::strings& s, int64_t size,
int64_t offset) {
- RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+ Status AppendUtf8Strings(const cpp11::strings& s, int64_t size, int64_t
offset) {
const SEXP* p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- // we know all the R strings are utf8 already, so we can get
- // a definite size and then use UnsafeAppend*()
- int64_t total_length = 0;
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- total_length += si == NA_STRING ? 0 : LENGTH(si);
- }
- RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+ if constexpr (is_string_view_type<T>::value) {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));
Review Comment:
Apologies for such an AI-written PR, but I'm a bit out of my depth here. I
don't know the answer to this but had Claude deep dive on it, and it says 🤖:
The contract used throughout `r_to_arrow.cpp` is that `Extend(x, size,
offset)` appends the elements in `[offset, size)`, i.e. `size - offset` values.
Every other converter in the file does `Reserve(size - offset)` and iterates
from `x + offset` (`RVectorIterator(x, start)` sets `ptr_x_ = DATAPTR_RO(x) +
start`; see e.g. the null, list and date converters). So `size - offset` here
is consistent with the rest of the file.
The pre-existing `String` path was the odd one out: it did
`Reserve(s.size())` and started reading from index 0 while looping `i` from
`offset`, so it would have appended the wrong `size - offset` elements for a
nonzero `offset`. That's latent because every caller (`DelayedExtend`) passes
`offset = 0`. I've now hoisted a single `Reserve(size - offset)` above the `if
constexpr` and switched all loops to `p_strings[i]` indexing, so both branches
honour `offset` the same way.
##########
r/src/r_to_arrow.cpp:
##########
@@ -862,54 +862,97 @@ class RPrimitiveConverter<T, enable_if_t<std::is_same<T,
FixedSizeBinaryType>::v
};
template <typename T>
-class RPrimitiveConverter<T, enable_if_string_like<T>>
+class RPrimitiveConverter<
+ T, enable_if_t<is_string_like_type<T>::value ||
is_string_view_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
public:
- using OffsetType = typename T::offset_type;
-
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
RVectorType rtype = GetVectorType(x);
if (rtype != STRING) {
return Status::Invalid("Expecting a character vector");
}
- return UnsafeAppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
+ return AppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
}
void DelayedExtend(SEXP values, int64_t size, RTasks& tasks) override {
auto task = [this, values, size]() { return this->Extend(values, size); };
- // TODO: refine this., e.g. extract setup from Extend()
tasks.Append(false, std::move(task));
}
private:
- Status UnsafeAppendUtf8Strings(const cpp11::strings& s, int64_t size,
int64_t offset) {
- RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+ Status AppendUtf8Strings(const cpp11::strings& s, int64_t size, int64_t
offset) {
const SEXP* p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- // we know all the R strings are utf8 already, so we can get
- // a definite size and then use UnsafeAppend*()
- int64_t total_length = 0;
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- total_length += si == NA_STRING ? 0 : LENGTH(si);
- }
- RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+ if constexpr (is_string_view_type<T>::value) {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));
- // append
- p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- if (si == NA_STRING) {
- this->primitive_builder_->UnsafeAppendNull();
- } else {
- this->primitive_builder_->UnsafeAppend(CHAR(si), LENGTH(si));
+ // Use safe Append (not UnsafeAppend) because StringView's heap builder
+ // has a 2GB-per-block limit that prevents bulk data pre-reservation.
+ p_strings += offset;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
+ if (si == NA_STRING) {
+ this->primitive_builder_->UnsafeAppendNull();
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Append(CHAR(si),
LENGTH(si)));
+ }
+ }
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
Review Comment:
Done, moved above the `if constexpr`.
##########
r/src/r_to_arrow.cpp:
##########
@@ -862,54 +862,97 @@ class RPrimitiveConverter<T, enable_if_t<std::is_same<T,
FixedSizeBinaryType>::v
};
template <typename T>
-class RPrimitiveConverter<T, enable_if_string_like<T>>
+class RPrimitiveConverter<
+ T, enable_if_t<is_string_like_type<T>::value ||
is_string_view_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
public:
- using OffsetType = typename T::offset_type;
-
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
RVectorType rtype = GetVectorType(x);
if (rtype != STRING) {
return Status::Invalid("Expecting a character vector");
}
- return UnsafeAppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
+ return AppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
}
void DelayedExtend(SEXP values, int64_t size, RTasks& tasks) override {
auto task = [this, values, size]() { return this->Extend(values, size); };
- // TODO: refine this., e.g. extract setup from Extend()
tasks.Append(false, std::move(task));
}
private:
- Status UnsafeAppendUtf8Strings(const cpp11::strings& s, int64_t size,
int64_t offset) {
- RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+ Status AppendUtf8Strings(const cpp11::strings& s, int64_t size, int64_t
offset) {
const SEXP* p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- // we know all the R strings are utf8 already, so we can get
- // a definite size and then use UnsafeAppend*()
- int64_t total_length = 0;
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- total_length += si == NA_STRING ? 0 : LENGTH(si);
- }
- RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+ if constexpr (is_string_view_type<T>::value) {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));
- // append
- p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- if (si == NA_STRING) {
- this->primitive_builder_->UnsafeAppendNull();
- } else {
- this->primitive_builder_->UnsafeAppend(CHAR(si), LENGTH(si));
+ // Use safe Append (not UnsafeAppend) because StringView's heap builder
+ // has a 2GB-per-block limit that prevents bulk data pre-reservation.
+ p_strings += offset;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
Review Comment:
Done.
##########
r/src/r_to_arrow.cpp:
##########
@@ -862,54 +862,97 @@ class RPrimitiveConverter<T, enable_if_t<std::is_same<T,
FixedSizeBinaryType>::v
};
template <typename T>
-class RPrimitiveConverter<T, enable_if_string_like<T>>
+class RPrimitiveConverter<
+ T, enable_if_t<is_string_like_type<T>::value ||
is_string_view_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
public:
- using OffsetType = typename T::offset_type;
-
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
RVectorType rtype = GetVectorType(x);
if (rtype != STRING) {
return Status::Invalid("Expecting a character vector");
}
- return UnsafeAppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
+ return AppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
}
void DelayedExtend(SEXP values, int64_t size, RTasks& tasks) override {
auto task = [this, values, size]() { return this->Extend(values, size); };
- // TODO: refine this., e.g. extract setup from Extend()
tasks.Append(false, std::move(task));
}
private:
- Status UnsafeAppendUtf8Strings(const cpp11::strings& s, int64_t size,
int64_t offset) {
- RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+ Status AppendUtf8Strings(const cpp11::strings& s, int64_t size, int64_t
offset) {
const SEXP* p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- // we know all the R strings are utf8 already, so we can get
- // a definite size and then use UnsafeAppend*()
- int64_t total_length = 0;
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- total_length += si == NA_STRING ? 0 : LENGTH(si);
- }
- RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+ if constexpr (is_string_view_type<T>::value) {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));
- // append
- p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- if (si == NA_STRING) {
- this->primitive_builder_->UnsafeAppendNull();
- } else {
- this->primitive_builder_->UnsafeAppend(CHAR(si), LENGTH(si));
+ // Use safe Append (not UnsafeAppend) because StringView's heap builder
+ // has a 2GB-per-block limit that prevents bulk data pre-reservation.
+ p_strings += offset;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
+ if (si == NA_STRING) {
+ this->primitive_builder_->UnsafeAppendNull();
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Append(CHAR(si),
LENGTH(si)));
+ }
+ }
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+
+ // We know all the R strings are utf8 already, so we can get
+ // a definite size and then use UnsafeAppend*()
+ int64_t total_length = 0;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
Review Comment:
Done.
##########
r/src/r_to_arrow.cpp:
##########
@@ -862,54 +862,97 @@ class RPrimitiveConverter<T, enable_if_t<std::is_same<T,
FixedSizeBinaryType>::v
};
template <typename T>
-class RPrimitiveConverter<T, enable_if_string_like<T>>
+class RPrimitiveConverter<
+ T, enable_if_t<is_string_like_type<T>::value ||
is_string_view_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
public:
- using OffsetType = typename T::offset_type;
-
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
RVectorType rtype = GetVectorType(x);
if (rtype != STRING) {
return Status::Invalid("Expecting a character vector");
}
- return UnsafeAppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
+ return AppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
}
void DelayedExtend(SEXP values, int64_t size, RTasks& tasks) override {
auto task = [this, values, size]() { return this->Extend(values, size); };
- // TODO: refine this., e.g. extract setup from Extend()
tasks.Append(false, std::move(task));
}
private:
- Status UnsafeAppendUtf8Strings(const cpp11::strings& s, int64_t size,
int64_t offset) {
- RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+ Status AppendUtf8Strings(const cpp11::strings& s, int64_t size, int64_t
offset) {
const SEXP* p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- // we know all the R strings are utf8 already, so we can get
- // a definite size and then use UnsafeAppend*()
- int64_t total_length = 0;
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- total_length += si == NA_STRING ? 0 : LENGTH(si);
- }
- RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+ if constexpr (is_string_view_type<T>::value) {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));
- // append
- p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- if (si == NA_STRING) {
- this->primitive_builder_->UnsafeAppendNull();
- } else {
- this->primitive_builder_->UnsafeAppend(CHAR(si), LENGTH(si));
+ // Use safe Append (not UnsafeAppend) because StringView's heap builder
+ // has a 2GB-per-block limit that prevents bulk data pre-reservation.
+ p_strings += offset;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
+ if (si == NA_STRING) {
+ this->primitive_builder_->UnsafeAppendNull();
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Append(CHAR(si),
LENGTH(si)));
+ }
+ }
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+
+ // We know all the R strings are utf8 already, so we can get
+ // a definite size and then use UnsafeAppend*()
+ int64_t total_length = 0;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
+ total_length += si == NA_STRING ? 0 : LENGTH(si);
+ }
+ RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+
+ p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
Review Comment:
Removed.
##########
r/src/r_to_arrow.cpp:
##########
@@ -862,54 +862,97 @@ class RPrimitiveConverter<T, enable_if_t<std::is_same<T,
FixedSizeBinaryType>::v
};
template <typename T>
-class RPrimitiveConverter<T, enable_if_string_like<T>>
+class RPrimitiveConverter<
+ T, enable_if_t<is_string_like_type<T>::value ||
is_string_view_type<T>::value>>
: public PrimitiveConverter<T, RConverter> {
public:
- using OffsetType = typename T::offset_type;
-
Status Extend(SEXP x, int64_t size, int64_t offset = 0) override {
RVectorType rtype = GetVectorType(x);
if (rtype != STRING) {
return Status::Invalid("Expecting a character vector");
}
- return UnsafeAppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
+ return AppendUtf8Strings(arrow::r::utf8_strings(x), size, offset);
}
void DelayedExtend(SEXP values, int64_t size, RTasks& tasks) override {
auto task = [this, values, size]() { return this->Extend(values, size); };
- // TODO: refine this., e.g. extract setup from Extend()
tasks.Append(false, std::move(task));
}
private:
- Status UnsafeAppendUtf8Strings(const cpp11::strings& s, int64_t size,
int64_t offset) {
- RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+ Status AppendUtf8Strings(const cpp11::strings& s, int64_t size, int64_t
offset) {
const SEXP* p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- // we know all the R strings are utf8 already, so we can get
- // a definite size and then use UnsafeAppend*()
- int64_t total_length = 0;
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- total_length += si == NA_STRING ? 0 : LENGTH(si);
- }
- RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+ if constexpr (is_string_view_type<T>::value) {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));
- // append
- p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
- for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
- SEXP si = *p_strings;
- if (si == NA_STRING) {
- this->primitive_builder_->UnsafeAppendNull();
- } else {
- this->primitive_builder_->UnsafeAppend(CHAR(si), LENGTH(si));
+ // Use safe Append (not UnsafeAppend) because StringView's heap builder
+ // has a 2GB-per-block limit that prevents bulk data pre-reservation.
+ p_strings += offset;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
+ if (si == NA_STRING) {
+ this->primitive_builder_->UnsafeAppendNull();
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Append(CHAR(si),
LENGTH(si)));
+ }
+ }
+ } else {
+ RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));
+
+ // We know all the R strings are utf8 already, so we can get
+ // a definite size and then use UnsafeAppend*()
+ int64_t total_length = 0;
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
+ total_length += si == NA_STRING ? 0 : LENGTH(si);
+ }
+ RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
+
+ p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));
+ for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
+ SEXP si = *p_strings;
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
