paleolimbot commented on code in PR #582:
URL: https://github.com/apache/arrow-nanoarrow/pull/582#discussion_r1714223621
##########
src/nanoarrow/ipc/encoder.c:
##########
@@ -78,30 +78,33 @@ void ArrowIpcEncoderReset(struct ArrowIpcEncoder* encoder) {
memset(encoder, 0, sizeof(struct ArrowIpcEncoder));
}
+static ArrowErrorCode ArrowIpcEncoderWriteContinuationAndSize(struct
ArrowBuffer* out,
+ size_t size) {
+ _NANOARROW_CHECK_UPPER_LIMIT(size, INT32_MAX);
+ int32_t header[] = {-1, ArrowIpcSystemEndianness() ==
NANOARROW_IPC_ENDIANNESS_BIG
+ ? (int32_t)bswap32((uint32_t)size)
+ : (int32_t)size};
Review Comment:
nit: If you can avoid the ternary operator, I find
```c
if (ArrowIpcSystemEndianness() == NANOARROW_IPC_ENDIANNESS_BIG) {
// ...
} else {
// ...
}
```
much easier to parse
##########
src/nanoarrow/ipc/encoder.c:
##########
@@ -78,30 +78,33 @@ void ArrowIpcEncoderReset(struct ArrowIpcEncoder* encoder) {
memset(encoder, 0, sizeof(struct ArrowIpcEncoder));
}
+static ArrowErrorCode ArrowIpcEncoderWriteContinuationAndSize(struct
ArrowBuffer* out,
+ size_t size) {
+ _NANOARROW_CHECK_UPPER_LIMIT(size, INT32_MAX);
+ int32_t header[] = {-1, ArrowIpcSystemEndianness() ==
NANOARROW_IPC_ENDIANNESS_BIG
+ ? (int32_t)bswap32((uint32_t)size)
+ : (int32_t)size};
+ return ArrowBufferAppend(out, &header, sizeof(header));
Review Comment:
Should this be `header` or `&header`?
##########
src/nanoarrow/ipc/files_test.cc:
##########
@@ -250,20 +250,33 @@ class TestFile {
// Read the same file with Arrow C++
auto maybe_table_arrow =
ReadTable(io::ReadableFile::Open(path_builder.str()));
- FAIL_RESULT_NOT_OK(maybe_table_arrow);
-
- AssertEqualsTable(std::move(schema), std::move(arrays),
- maybe_table_arrow.ValueUnsafe());
+ {
+ SCOPED_TRACE("Read the same file with Arrow C++");
+ FAIL_RESULT_NOT_OK(maybe_table_arrow);
+ AssertEqualsTable(std::move(schema), std::move(arrays),
+ maybe_table_arrow.ValueUnsafe());
+ }
- // Read the roundtripped buffer using nanoarrow
nanoarrow::UniqueSchema roundtripped_schema;
std::vector<nanoarrow::UniqueArray> roundtripped_arrays;
- ASSERT_EQ(ReadArrowArrayStreamIPC(dir_prefix, roundtripped_schema.get(),
- &roundtripped_arrays, &error),
- NANOARROW_OK);
+ {
+ SCOPED_TRACE("Read the roundtripped buffer using nanoarrow");
+ ASSERT_EQ(ReadArrowArrayStreamIPC(dir_prefix, roundtripped_schema.get(),
+ &roundtripped_arrays, &error),
Review Comment:
Should this line refer to the buffer `roundtripped`? (i.e., maybe the
previous test was just reading from the file and not the roundtripped data?)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
