Copilot commented on code in PR #49712:
URL: https://github.com/apache/arrow/pull/49712#discussion_r3436861512
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -826,7 +826,8 @@ class PyDictionaryConverter<U, enable_if_has_string_view<U>>
} else {
ARROW_RETURN_NOT_OK(
PyValue::Convert(this->value_type_, this->options_, value, view_));
- return this->value_builder_->Append(view_.bytes,
static_cast<int32_t>(view_.size));
+ return this->value_builder_->Append(
+ std::string_view(view_.bytes, static_cast<size_t>(view_.size)));
}
Review Comment:
`std::string_view` is introduced here, but this translation unit doesn’t
include `<string_view>`. Depending on transitive includes is not portable and
can break builds on some toolchains; please add `#include <string_view>` to the
include list near the top of the file.
##########
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
GetString().
+ if constexpr (!std::is_same_v<StringArrayType, arrow::StringViewArray>) {
Review Comment:
The comment says this “falls through to GetString()”, but the implementation
below uses `GetView()` and `r_string_from_view*()`. Keeping the comment
accurate helps future maintainers understand why the fast path is skipped for
StringViewArray.
--
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]
