GH-50251: Add GetSpan() convenience method to ArrayData#50310
GH-50251: Add GetSpan() convenience method to ArrayData#50310VedantRalekar wants to merge 2 commits into
Conversation
|
|
| return std::span<const T>( | ||
| reinterpret_cast<const T*>(buffers[i]->data()) + offset, length); | ||
| } |
There was a problem hiding this comment.
Thanks for pr, @VedantRalekar!
These lines need clang-format fix per Dev / Lint job logs.
See also Styling and Dev Guidelines, you can run
pre-commit run --show-diff-on-failure --color=always --all-files cpp
One optional point to consider - perhaps add a test in array_test.cc to cover + offset and also the new null-buffer case?
There was a problem hiding this comment.
Thanks for the feedback! I'll apply the clang-format fixes and add the suggested tests for the offset and null-buffer cases. I'll update the PR once the changes are ready.
| /// \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 |
There was a problem hiding this comment.
Maybe add a mention of empty span to \return
There was a problem hiding this comment.
i have added a mention of empty span to \return, please review it.
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for your contribution @VedantRalekar , please find some comments below.
| /// this array type | ||
| /// \return A span<const T> of the requested length, or empty if buffers[i] is null | ||
| template <typename T> | ||
| std::span<const T> GetSpan(int i, int64_t length) const { |
There was a problem hiding this comment.
We have ArrayData::GetMutableValues in addition to ArrayData::GetValues, why not add GetMutableSpan as well?
|
|
||
| // Buffer with values 0..9 | ||
| std::vector<T> values = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; | ||
| auto value_buffer = Buffer::FromVector(values).ValueOrDie(); |
There was a problem hiding this comment.
Can use ASSERT_OK_AND_ASSIGN instead of dying on error
| ASSERT_EQ(span.size(), length); | ||
| ASSERT_EQ(span[0], 3); // values[offset] | ||
| ASSERT_EQ(span[length - 1], 6); // values[offset + length - 1] | ||
| ASSERT_EQ(span.data(), value_buffer->data_as<T>() + offset); |
There was a problem hiding this comment.
Can you also exercise calling GetSpan with a smaller length?
| auto data = ArrayData::Make(int64(), length, {nullptr, nullptr}, | ||
| /*null_count=*/0, offset); | ||
| auto span = data->GetSpan<T>(/*i=*/1, length); | ||
| ASSERT_TRUE(span.empty()); | ||
| ASSERT_EQ(span.data(), nullptr); |
There was a problem hiding this comment.
This is invalid. All null buffers in an integer array is only supported for length=0, can you fix the test accordingly?
| // --- Case B: Null bitmap is nullptr, values buffer valid → span works --- | ||
| { | ||
| auto data = ArrayData::Make(int64(), length, {nullptr, value_buffer}, | ||
| /*null_count=*/0, offset); | ||
| auto span = data->GetSpan<T>(/*i=*/1, length); | ||
| ASSERT_EQ(span.size(), length); | ||
| ASSERT_EQ(span[0], 10); | ||
| ASSERT_EQ(span[3], 40); | ||
| } |
There was a problem hiding this comment.
You're already testing this in GetSpanWithOffset, aren't you?
|
Closing as superseded by #50366 |
3 participants
What changes were proposed in this pull request?
This PR adds a
GetSpan()convenience method toArrayData, similar to the existingGetSpan()implementation inArraySpan.Why are the changes needed?
ArraySpanalready provides a convenient way to access buffer data asstd::span, whileArrayDatadoes not. This change adds a similar API to improve consistency and make span-based buffer access easier.Fixes #50251.