You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by GitBox <gi...@apache.org> on 2022/08/10 19:05:02 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #17: Buffer element appenders
paleolimbot opened a new pull request, #17:
URL: https://github.com/apache/arrow-nanoarrow/pull/17
It turns out this is really annoying to do otherwise! Declaring a variable of an appropriate type gets verbose when switching on type, and it sounds like these functions might be useful for database drivers, too.
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #17: Buffer element appenders
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on code in PR #17:
URL: https://github.com/apache/arrow-nanoarrow/pull/17#discussion_r943029428
##########
src/nanoarrow/buffer_test.cc:
##########
@@ -160,3 +160,31 @@ TEST(BufferTest, BufferTestError) {
ArrowBufferReset(&buffer);
}
+
+TEST(BufferTest, BufferTestAppendHelpers) {
+ struct ArrowBuffer buffer;
+ ArrowBufferInit(&buffer);
+
+ EXPECT_EQ(ArrowBufferAppendInt8(&buffer, 123), NANOARROW_OK);
+ EXPECT_EQ(reinterpret_cast<int8_t*>(buffer.data + buffer.size_bytes)[-1], 123);
+ EXPECT_EQ(ArrowBufferAppendUInt8(&buffer, 123), NANOARROW_OK);
+ EXPECT_EQ(reinterpret_cast<uint8_t*>(buffer.data + buffer.size_bytes)[-1], 123);
+ EXPECT_EQ(ArrowBufferAppendInt16(&buffer, 123), NANOARROW_OK);
Review Comment:
It's easy enough to reset!
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] lidavidm commented on a diff in pull request #17: Buffer element appenders
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #17:
URL: https://github.com/apache/arrow-nanoarrow/pull/17#discussion_r942812080
##########
src/nanoarrow/buffer_test.cc:
##########
@@ -160,3 +160,31 @@ TEST(BufferTest, BufferTestError) {
ArrowBufferReset(&buffer);
}
+
+TEST(BufferTest, BufferTestAppendHelpers) {
+ struct ArrowBuffer buffer;
+ ArrowBufferInit(&buffer);
+
+ EXPECT_EQ(ArrowBufferAppendInt8(&buffer, 123), NANOARROW_OK);
+ EXPECT_EQ(reinterpret_cast<int8_t*>(buffer.data + buffer.size_bytes)[-1], 123);
+ EXPECT_EQ(ArrowBufferAppendUInt8(&buffer, 123), NANOARROW_OK);
+ EXPECT_EQ(reinterpret_cast<uint8_t*>(buffer.data + buffer.size_bytes)[-1], 123);
+ EXPECT_EQ(ArrowBufferAppendInt16(&buffer, 123), NANOARROW_OK);
Review Comment:
I think these are all unaligned accesses; that's probably OK, but I think Arrow itself tries to avoid this (or uses memcpy to do unaligned reads) to be portable. That said this is test code so not a big deal.
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot commented on pull request #17: Buffer element appenders
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on PR #17:
URL: https://github.com/apache/arrow-nanoarrow/pull/17#issuecomment-1211464135
Ok - this is a first pass at #8 that implements the functions needed to make "build by buffer" a thing. The `ArrowBufferAppendInt8()` family of functions helps make code that does `switch(some_arrow_type) { ... }` prettier. Rather than make `ArrowArrayAppendInt8()`, this just exposes the internal buffer objects and lets you do all the buffer method things you need to do (requiring some knowledge of the spec). Rowwise builders can cache the arrow type and do `switch (the_arrow_type) { ... }` and append accordingly.
Most of this was implemented first in #16 but it sounds like many of those changes aren't required to get started on database driver work. This PR is intended to be the minimum change needed to facilitate that work.
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot merged pull request #17: Buffer element appenders
Posted by GitBox <gi...@apache.org>.
paleolimbot merged PR #17:
URL: https://github.com/apache/arrow-nanoarrow/pull/17
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] codecov-commenter commented on pull request #17: Buffer element appenders
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #17:
URL: https://github.com/apache/arrow-nanoarrow/pull/17#issuecomment-1211146044
# [Codecov](https://codecov.io/gh/apache/arrow-nanoarrow/pull/17?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17](https://codecov.io/gh/apache/arrow-nanoarrow/pull/17?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (673a22c) into [main](https://codecov.io/gh/apache/arrow-nanoarrow/commit/df62a5e1799892593698990773f768a37927dae9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (df62a5e) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #17 +/- ##
=======================================
Coverage 90.64% 90.64%
=======================================
Files 9 9
Lines 1037 1037
Branches 43 43
=======================================
Hits 940 940
Misses 63 63
Partials 34 34
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-nanoarrow/pull/17?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/nanoarrow/buffer\_inline.h](https://codecov.io/gh/apache/arrow-nanoarrow/pull/17/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9idWZmZXJfaW5saW5lLmg=) | `84.78% <ø> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org