feat: symbolically represent IPC message bodies in IR - #115
feat: symbolically represent IPC message bodies in IR#115Benjamin-Philip wants to merge 9 commits into
Conversation
|
Preview URL: https://Benjamin-Philip.github.io/arrow-erlang-1 If the preview URL doesn't work, you may have forgotten to configure your fork repository for preview. |
There was a problem hiding this comment.
Pull request overview
Updates the IPC message IR so RecordBatch message bodies are kept as symbolic Arrow arrays (to support Arrow JSON) and are only serialized when producing IPC (EMF) bytes.
Changes:
- Store RecordBatch message bodies as
[arrow_array:array()]in the IR and serialize them into_ipc/1. - Add
arrow_ipc_record_batch:body_length/1and use it to populate#message.body_length. - Update test fixtures and suites to expect symbolic bodies and validate computed body length.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/arrow_ipc_record_batch_SUITE.erl | Adds a new test asserting the computed RecordBatch body length matches the serialized body size. |
| test/arrow_ipc_message_SUITE.erl | Updates expectations so RecordBatch message bodies remain symbolic (arrays) in the IR. |
| test/arrow_ipc_marks_data.hrl | Updates test macros to construct RecordBatch messages with symbolic bodies (columns) instead of pre-serialized binaries. |
| src/arrow_ipc_record_batch.erl | Exports and implements body_length/1 for RecordBatch bodies. |
| src/arrow_ipc_message.hrl | Changes #message.body type to `[arrow_array:array()] |
| src/arrow_ipc_message.erl | Updates message construction and IPC serialization to serialize body arrays at to_ipc/1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/arrow_ipc_message.erl:36
- The module doc still describes
#message.bodyas a singlearrow_array:array/0, but the record field andfrom_erlang/2now use a list of arrays. This mismatch can confuse users and will make the docs inconsistent with the new symbolic representation approach.
5. `body`: The actual body. Can be undefined (in the case of Schema)
or a `t:arrow_array:array/0` (in the case of Record Batch).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/arrow_ipc_message.erl:76
from_erlang/1is now specified/documented as taking only a schema header, but the implementation will still accept any term (including a record batch header) and silently produce a message withbody_length = 0and no body. That can lead to invalid IPC messages that are hard to debug; it’s safer to reject non-schema headers at runtime.
from_erlang(Header) ->
#message{header = Header, body_length = 0}.
src/arrow_ipc_message.erl:102
to_ipc/1serializes the Flatbuffers metadata before computing the body binary, and relies on theMessage#message.body_lengthalready being correct. Sincefrom_erlang/2now accepts a symbolic body (arrays) and computesbody_lengthfrom the record batch header, it’s possible forbody_lengthto drift from the actual serialized body size (or for callers to accidentally pass a mismatched header/body). Compute the body first and setbody_lengthfrombyte_size(Body)when callingarrow_format_nif:serialize_message/1so the EMF metadata stays consistent with the emitted bytes.
Metadata = arrow_format_nif:serialize_message(Message#message{body = undefined}),
MetadataSize = <<(byte_size(Metadata)):32>>,
Body =
case Message#message.body of
undefined ->
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/arrow_ipc_message.erl:105
to_ipc/1now assumesMessage#message.bodyis always a list of arrays and will crash withbadargif a caller constructs a#message{body = Bin}(binary) directly (which previously worked). If you want this change to be strictly backward compatible and/or produce a clearer error, add guards for list vs binary (and a fallback error) in the body serialization case expression.
case Message#message.body of
undefined ->
<<>>;
Arrays ->
body_from_erlang(Arrays)
What issue does this PR close?
Closes #112.
The body of a message needs to represented symbolically in Arrow JSON. We
previously serialized it within our internal representation, making it
impossible to produce Arrow JSON from the IR.
What's Changed
We now leave the body as-is in the IR and only serialize it on
to_ipc.