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?
| // --- Case C: Null bitmap exists and valid, values buffer valid → span works --- | ||
| { | ||
| // All bits set = no nulls | ||
| auto null_buffer = *Buffer::FromString(std::string("\xFF\xFF\xFF\xFF", 4)); | ||
| auto data = ArrayData::Make(int64(), length, {null_buffer, value_buffer}, | ||
| /*null_count=*/0, offset); | ||
| auto span = data->GetSpan<T>(/*i=*/1, length); | ||
| // The span ignores the null bitmap entirely – it just looks at buffer index 1 | ||
| ASSERT_EQ(span.size(), length); | ||
| ASSERT_EQ(span[0], 10); | ||
| ASSERT_EQ(span[3], 40); | ||
| } |
There was a problem hiding this comment.
This test doesn't seem useful either.
(also, are you generating these tests with AI? did you review them before pushing?)
There was a problem hiding this comment.
yeah , but i didn't understand that cases thats why used ai and i will try to fix the tests and will also add the getmutablespan, could you please give me bit more idea about test in array_test.cc to cover + offset and also the new null-buffer case.
There was a problem hiding this comment.
Well, our AI contribution policy states that contributor should only use AI when they are able to understand the changes themselves otherwise it adds unnecessary extra burden for maintainers.
What I would recommend is either having the AI explain to you the changes step by step, or finding another ticket to work on that you have a better understanding of.
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.