Copilot commented on code in PR #115:
URL: https://github.com/apache/arrow-erlang/pull/115#discussion_r3701719536
##########
src/arrow_ipc_message.erl:
##########
@@ -33,7 +33,7 @@ to represent a message. Metadata such as:
3. `body_length`: The length of the body in bytes
4. `custom_metadata`: A list of custom metadata in key-value format
5. `body`: The actual body. Can be undefined (in the case of Schema)
- or a binary (in the case of Record Batch).
+ or a `t:arrow_array:array/0` (in the case of Record Batch).
Review Comment:
Module documentation says the message body is a single
`t:arrow_array:array/0` for record batches, but the record/type has been
changed to store a *list* of arrays (`[arrow_array:array()]`). Updating this
wording avoids misleading API docs for callers.
##########
src/arrow_ipc_record_batch.erl:
##########
@@ -114,3 +114,12 @@ buffer_data(Buffer, CurOffset) ->
#{offset => CurOffset, length => Buffer#buffer.length},
arrow_buffer:size(Buffer) + CurOffset
}.
+
+-doc """
+Returns the body length of a Record Batch.
+""".
+-spec body_length(RecordBatch :: record_batch()) -> non_neg_integer().
+body_length(RecordBatch) ->
+ Buffers = RecordBatch#record_batch.buffers,
+ #{offset := Offset, length := Length} = lists:last(Buffers),
+ Offset + Length + (64 - (Length rem 64)).
Review Comment:
body_length/1 currently computes padding as (64 - (Length rem 64)), which
adds an extra 64 bytes when Length is already 64-byte aligned (Length rem 64
=:= 0). It also bases padding on Length rather than the end offset, which can
produce incorrect totals if offsets ever deviate from 64-byte alignment. Use
arrow_utils:pad_len/1 to compute the padding (0 when already aligned) and add
it to the end position.
##########
src/arrow_ipc_message.erl:
##########
@@ -68,21 +68,22 @@ for more info:
-type key_value() :: #{key => string(), value => string()}.
-doc """
-Creates a message given a data header.
+Creates a message given a schema data header.
""".
--spec from_erlang(Header :: arrow_ipc_schema:schema() |
arrow_ipc_record_batch:record_batch()) ->
+-spec from_erlang(Header :: arrow_ipc_schema:schema()) ->
Message :: message().
from_erlang(Header) ->
#message{header = Header, body_length = 0}.
-doc """
-Creates a message given a data header and a body.
+Creates a message given a record batch data header and a body.
""".
-spec from_erlang(
- Header :: arrow_ipc_schema:schema() |
arrow_ipc_record_batch:record_batch(), Body :: binary()
+ Header :: arrow_ipc_record_batch:record_batch(),
+ Body :: [arrow_array:array()]
) -> Message :: message().
from_erlang(Header, Body) ->
- #message{header = Header, body = Body, body_length = byte_size(Body)}.
+ #message{header = Header, body = Body, body_length =
arrow_ipc_record_batch:body_length(Header)}.
Review Comment:
This change makes from_erlang/2 store the body as `[arrow_array:array()]`
(and to_ipc/1 always calls body_from_erlang/1). There are still documented
usages that pass a pre-serialized binary body (e.g.
guides/quick-run-through.livemd:173-175), which will now crash at runtime when
to_ipc/1 calls body_from_erlang/1 with a binary. Please update those call
sites/docs to pass the columns (arrays) instead of the binary body.
--
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]
