thisisnic commented on code in PR #49712:
URL: https://github.com/apache/arrow/pull/49712#discussion_r4524555753
##########
r/src/array_to_vector.cpp:
##########
@@ -290,26 +290,32 @@ struct Converter_String : public Converter {
Status Ingest_some_nulls(SEXP data, const std::shared_ptr<arrow::Array>&
array,
R_xlen_t start, R_xlen_t n, size_t chunk_index)
const {
- auto p_offset = array->data()->GetValues<int32_t>(1);
- if (!p_offset) {
- return Status::Invalid("Invalid offset buffer");
- }
- auto p_strings = array->data()->GetValues<char>(2, *p_offset);
- if (!p_strings) {
- // There is an offset buffer, but the data buffer is null
- // There is at least one value in the array and not all the values are
null
- // That means all values are either empty strings or nulls so there is
nothing to do
-
- if (array->null_count()) {
- 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.IsNotSet()) {
- SET_STRING_ELT(data, start + i, NA_STRING);
+ // StringViewArray uses a different memory layout (views + data buffers)
rather
+ // than offsets, so skip the offset-based fast path and fall through to the
+ // GetView()-based element loop below.
+ if (array->type_id() != Type::STRING_VIEW) {
+ auto p_offset = array->data()->GetValues<int32_t>(1);
Review Comment:
Yeah, I think you're right. As far as I can tell that line was reading the
offsets as `int32_t` regardless of type, so I've switched it to
`StringArrayType::offset_type`, which should give LargeString its 64-bit
offsets.
--
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]
