Copilot commented on code in PR #49712:
URL: https://github.com/apache/arrow/pull/49712#discussion_r3437460637
##########
r/src/array_to_vector.cpp:
##########
@@ -290,26 +290,31 @@ 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.
+ auto p_offset = array->data()->GetValues<int32_t>(1);
Review Comment:
`Converter_String::Ingest_some_nulls()` now has an unconditional
offset-buffer fast path plus a closing brace, but there is no opening
conditional/scope. This both breaks compilation (unmatched `}`) and also
contradicts the intent in the comment: for `StringViewArray` the offset-buffer
path must be skipped because buffer(1) is views, not 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]
