fix(cpp): Fix offset handling in ViewArrayAs Range Helpers #702
Conversation
| @@ -157,6 +158,7 @@ class ViewArrayAs { | |||
| array_view->buffer_views[1].data.data, | |||
| array_view->offset, | |||
| }, | |||
| array_view->offset, | |||
There was a problem hiding this comment.
This is an easy fix but maybe not the API we want, since we are providing the offset to both the Range and the Get class template (?)
There was a problem hiding this comment.
This seems like the right thing to put here (at least one of Range or Get need it?)
| @@ -157,6 +158,7 @@ class ViewArrayAs { | |||
| array_view->buffer_views[1].data.data, | |||
| array_view->offset, | |||
| }, | |||
| array_view->offset, | |||
There was a problem hiding this comment.
This seems like the right thing to put here (at least one of Range or Get need it?)
src/nanoarrow/hpp/view.hpp
Outdated
| @@ -79,7 +80,7 @@ struct RandomAccessRange { | |||
| }; | |||
|
|
|||
| const_iterator begin() const { return {0, this}; } | |||
| const_iterator end() const { return {size, this}; } | |||
| const_iterator end() const { return {size - offset, this}; } | |||
There was a problem hiding this comment.
I would have expected this to be:
const_iterator begin() const { return {offset, this}; }
const_iterator end() const { return {offset + size, this}; }...or to have it be [0, size] and to have the offset handled by range->get().
There was a problem hiding this comment.
Not quite what you asked for but I think I've simplified this a bit by taking the offset out of the Get class and making it so that the range bounds start at the offset and end at size
src/nanoarrow/hpp/view_test.cc
Outdated
| ArrowStringView expected[] = {"baz"_asv, "qux"_asv}; | ||
| int i = 0; | ||
| for (auto slot : nanoarrow::ViewArrayAsFixedSizeBytes(array.get(), FixedSize)) { | ||
| EXPECT_EQ(slot, expected[i]); | ||
| i++; | ||
| } |
There was a problem hiding this comment.
Can you use ::testing::ElementsAre("baz"_asv, "qux"_asv)? (Not sure if the templates work for string views but they might)
WillAyd
force-pushed
the
offset-array-bug
branch
from
2a73ad7 to
b23d177
Compare
|
Hmm I think I might be missing something simple with the offest. So when we have an array of |
I think you also need to decrement the |
WillAyd
changed the title
|
Awesome - thanks for the help on review |
closes #701