fix: IPC streams did not include RecordBatch headers #582
Conversation
bkietz
commented
Aug 12, 2024
bkietz
commented
- As noted feat: Add IPC stream writing #571 (comment) RecordBatch Messages were not being written during IPC streaming. This has been corrected
- Testing uses one more round trip case where we have arrow C++ read from a stream written by nanoarrow.
- During testing it also became apparent that encapsulated messages were not always stored with the correct size; metadata_size should include padding (but not the continuation or size itself) https://arrow.apache.org/docs/format/Columnar.html#encapsulated-message-format
bkietz
force-pushed
the
finalize-record-batch-headers
branch
from
914a2cc
to
7f32b76
Compare
There was a problem hiding this comment.
Thanks!
I am not sure that this is actually running the nanoarrow read tests against your encoder (although after this PR your encoder is getting checked via Arrow C++). It may be worth checking docker compose run --rm coverage (a quick check on main suggests that most of writer.c is never run during tests, although this may be the coverage setup).
src/nanoarrow/ipc/files_test.cc
Outdated
| ASSERT_EQ(ReadArrowArrayStreamIPC(dir_prefix, roundtripped_schema.get(), | ||
| &roundtripped_arrays, &error), |
There was a problem hiding this 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?)
There was a problem hiding this comment.
I think you're right. FWIW after reading from roundtripped instead of the file the tests still pass.
src/nanoarrow/ipc/encoder.c
Outdated
| int32_t header[] = {-1, ArrowIpcSystemEndianness() == NANOARROW_IPC_ENDIANNESS_BIG | ||
| ? (int32_t)bswap32((uint32_t)size) | ||
| : (int32_t)size}; | ||
| return ArrowBufferAppend(out, &header, sizeof(header)); |
There was a problem hiding this comment.
Should this be header or &header?
There was a problem hiding this comment.
header works because it's an array and arrays are convertible to pointer. I prefer &header because that would be correct even if header were not an array
(files_test is not included in the coverage setup, which might have been intentional since those are closer to integration tests than unit tests) |
bkietz
force-pushed
the
finalize-record-batch-headers
branch
from
7f32b76
to
962cfbd
Compare
