You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2024/09/25 19:11:52 UTC
[PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
WillAyd opened a new pull request, #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635
closes https://github.com/apache/arrow-nanoarrow/issues/634#issuecomment-2374774506
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1775842233
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3343,7 +3343,50 @@ TEST(ArrayViewTest, ArrayViewTestGetString) {
TestGetFromBinary<FixedSizeBinaryBuilder>(fixed_size_builder);
}
-template <typename BuilderClass>
+static const void* GetBinaryData(const struct ArrowBufferView* bv) {
Review Comment:
Open to different ideas on how to best template these tests; if constexpr would be really nice here!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1777790556
##########
src/nanoarrow/common/inline_array.h:
##########
@@ -525,6 +525,7 @@ static inline ArrowErrorCode ArrowArrayAddVariadicBuffers(struct ArrowArray* arr
private_data->variadic_buffer_sizes[i] = 0;
}
private_data->n_variadic_buffers = n_bufs_needed;
+ array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS + 1 + n_bufs_needed;
Review Comment:
For clarity, we might want to rename `n_bufs_needed` to `nvariadic_bufs_needed`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1777793760
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3343,8 +3387,49 @@ TEST(ArrayViewTest, ArrayViewTestGetString) {
TestGetFromBinary<FixedSizeBinaryBuilder>(fixed_size_builder);
}
-template <typename BuilderClass>
-void TestGetFromBinaryView(BuilderClass& builder) {
+template <typename BuilderClass, typename ValueT>
+void TestGetFromInlinedBinaryView(
Review Comment:
If you're up for it, having them both take a function parameter I think makes sense!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1777793549
##########
src/nanoarrow/common/inline_array.h:
##########
@@ -525,6 +525,7 @@ static inline ArrowErrorCode ArrowArrayAddVariadicBuffers(struct ArrowArray* arr
private_data->variadic_buffer_sizes[i] = 0;
}
private_data->n_variadic_buffers = n_bufs_needed;
+ array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS + 1 + n_bufs_needed;
Review Comment:
That seems reasonable to me if you're up for it!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1776324876
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3343,7 +3343,50 @@ TEST(ArrayViewTest, ArrayViewTestGetString) {
TestGetFromBinary<FixedSizeBinaryBuilder>(fixed_size_builder);
}
-template <typename BuilderClass>
+static const void* GetBinaryData(const struct ArrowBufferView* bv) {
Review Comment:
It's a personal preference, but I usually find that at the point I get to `if constexpr` I've missed a better way to abstract things. Some people love it though!
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3380,29 +3423,17 @@ void TestGetFromBinaryView(BuilderClass& builder) {
EXPECT_EQ(ArrowArrayViewIsNull(&array_view, 2), 1);
EXPECT_EQ(ArrowArrayViewIsNull(&array_view, 3), 0);
- auto string_view = ArrowArrayViewGetStringUnsafe(&array_view, 3);
Review Comment:
"We" is mostly just me here, but I think it would be strange to claim that our nanoarrow.hpp supports C++11 if we don't compile and run our tests using C++11. Part of being a vendorable library is inheriting the constraints of any project that might want to use you, which includes compiler warning policies, C++ standards, big endian support, etc.
Refactor definitely welcome! In the next release I think it will be a good opportunity to clean up tests.
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3343,7 +3343,50 @@ TEST(ArrayViewTest, ArrayViewTestGetString) {
TestGetFromBinary<FixedSizeBinaryBuilder>(fixed_size_builder);
}
-template <typename BuilderClass>
+static const void* GetBinaryData(const struct ArrowBufferView* bv) {
Review Comment:
Can you accept `std::function` or a function pointer as an argument? Like:
https://github.com/apache/arrow-nanoarrow/blob/53c9f8f69743d633b831b735de1402cca38713ad/src/nanoarrow/testing/testing_test.cc#L58-L63
```c
template <typename BuilderClass>
void TestGetFromInlinedBinaryView(BuilderClass& builder, enum ArrowType type_id, std::string (*GetBinaryDataAsString)(ArrowArrayView*, int64_t)) { ... }
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#issuecomment-2375834947
Good point - yes the builder starts with n_array = 2. I'll take a look at that
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1775883983
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3380,29 +3423,17 @@ void TestGetFromBinaryView(BuilderClass& builder) {
EXPECT_EQ(ArrowArrayViewIsNull(&array_view, 2), 1);
EXPECT_EQ(ArrowArrayViewIsNull(&array_view, 3), 0);
- auto string_view = ArrowArrayViewGetStringUnsafe(&array_view, 3);
Review Comment:
Also generally curious - why do we keep the test suite tied to C++11? Is that to let downstream libraries run the tests without imposing a more recent standard?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1775845690
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3380,29 +3423,17 @@ void TestGetFromBinaryView(BuilderClass& builder) {
EXPECT_EQ(ArrowArrayViewIsNull(&array_view, 2), 1);
EXPECT_EQ(ArrowArrayViewIsNull(&array_view, 3), 0);
- auto string_view = ArrowArrayViewGetStringUnsafe(&array_view, 3);
Review Comment:
A bit of a refactor mixed in, but I didn't want to keep repeating this pattern of testing both String/Byte get functions for all Ts
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1777789012
##########
src/nanoarrow/common/array.c:
##########
@@ -115,7 +115,7 @@ static ArrowErrorCode ArrowArraySetStorageType(struct ArrowArray* array,
break;
case NANOARROW_TYPE_BINARY_VIEW:
case NANOARROW_TYPE_STRING_VIEW:
- array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS;
+ array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS + 1;
Review Comment:
While this "works" I'm not sure if we still care to maintain this distinction of `NANOARROW_BINARY_FIXED_VIEW_BUFFERS` being 2 when all other types really have 3 fixed buffers. I suppose the last buffer isn't fixed, in the sense that it can be moved. So semantically maybe there is some value in having this still? But we could probably reconsolidate some code if we go back to just using `NANOARROW_MAX_FIXED_BUFFERS` like the other types
##########
src/nanoarrow/common/array_test.cc:
##########
@@ -3343,8 +3387,49 @@ TEST(ArrayViewTest, ArrayViewTestGetString) {
TestGetFromBinary<FixedSizeBinaryBuilder>(fixed_size_builder);
}
-template <typename BuilderClass>
-void TestGetFromBinaryView(BuilderClass& builder) {
+template <typename BuilderClass, typename ValueT>
+void TestGetFromInlinedBinaryView(
Review Comment:
There is some inconsistency between the signature of this test and `TestAppendToInlinedDataViewArray` preceding it. Whereas here we are accepting `GetValueFunc` and `GetValueDataFunc` as arguments, the aforementioned test still takes a template argument.
Should I just move them both to using a function argument instead of a template argument?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix; Properly ingest Binary View types without variadic buffers [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #635:
URL: https://github.com/apache/arrow-nanoarrow/pull/635#discussion_r1777794606
##########
src/nanoarrow/common/array.c:
##########
@@ -115,7 +115,7 @@ static ArrowErrorCode ArrowArraySetStorageType(struct ArrowArray* array,
break;
case NANOARROW_TYPE_BINARY_VIEW:
case NANOARROW_TYPE_STRING_VIEW:
- array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS;
+ array->n_buffers = NANOARROW_BINARY_VIEW_FIXED_BUFFERS + 1;
Review Comment:
I think it's still good to keep (you could leave a note along those lines as a comment if you'd like next to the definition)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org