felipecrv commented on code in PR #38027:
URL: https://github.com/apache/arrow/pull/38027#discussion_r1406323011
##########
cpp/src/arrow/array/data.h:
##########
@@ -434,6 +435,23 @@ struct ARROW_EXPORT ArraySpan {
return GetValues<T>(i, this->offset);
}
+ // Access a buffer's data as a span
+ template <typename T>
+ util::span<const T> GetSpan(int i, int64_t length) const {
+ int buffer_length = static_cast<size_t>(buffers[i].size) / sizeof(T);
+ assert(length > 0 && length + offset <= buffer_length);
Review Comment:
`0` is a valid length for some buffers, did you mean to check `i > 0`?
(because `0` is a bitmap buffer and can't be accessed as a `span`).
##########
cpp/src/arrow/array/data.h:
##########
@@ -434,6 +435,23 @@ struct ARROW_EXPORT ArraySpan {
return GetValues<T>(i, this->offset);
}
+ // Access a buffer's data as a span
+ template <typename T>
+ util::span<const T> GetSpan(int i, int64_t length) const {
+ int buffer_length = static_cast<size_t>(buffers[i].size) / sizeof(T);
+ assert(length > 0 && length + offset <= buffer_length);
+ util::span<const T> data_span(buffers[i].data_as<T>(), buffer_length);
+ return data_span.subspan(offset, length);
Review Comment:
`GetSpan` should and will be inlined a lot, and the pre-conditions (that
should be documented on the docstring) checked by the `assert` mean we don't
need the extra truncation logic that `subspan` has.
`return util::span<const T>(buffers[i].data_as<T>() + this->offset, length);`
##########
cpp/src/arrow/array/data.h:
##########
@@ -434,6 +435,23 @@ struct ARROW_EXPORT ArraySpan {
return GetValues<T>(i, this->offset);
}
+ // Access a buffer's data as a span
+ template <typename T>
+ util::span<const T> GetSpan(int i, int64_t length) const {
+ int buffer_length = static_cast<size_t>(buffers[i].size) / sizeof(T);
Review Comment:
`int` might not be enough to fit the number of elements in the buffer, so
this is better done with `int64_t` on all operands and the result:
```cpp
const int64_t buffer_length = buffers[i].size /
static_cast<int64_t>(sizeof(T));
```
You can cast it to `size_t` later if needed.
##########
cpp/src/arrow/array/data.h:
##########
@@ -434,6 +435,23 @@ struct ARROW_EXPORT ArraySpan {
return GetValues<T>(i, this->offset);
}
+ // Access a buffer's data as a span
Review Comment:
I think this function can benefit from a more ellaborate (in Doxygen format)
docstring.
```suggestion
/// \brief Access a buffer's data as a span
///
/// \param i The buffer index
/// \param length The required length (in number of typed values) of the
requested span
/// \pre i > 0
/// \pre length <= the length of the buffer (in number of values) that's
expected for this array type
/// \return A span<const T> of the requested length
```
--
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]
