fix: arrow_buffer:size/1 use 64 byte padding not 8

[Benjamin-Philip]

What issue does this PR close?

Closes #113.

Previously, arrow_buffer:size/1 took 8 byte padding to calculate size
when the buffers actually used 64 bytes. This resulted in various gross
errors in offsets and other metadata, rendering any IPC binaries
produced unusable.

What's Changed

arrow_buffer:size/1 now correctly uses 64 byte padding. Record Batch test data
has been updated with the correct offsets.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

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.
See https://github.com/apache/arrow-erlang/blob/main/CONTRIBUTING.md#forks for instructions on how to configure.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/arrow_buffer.erl:120

• arrow_buffer:size/1 can legally return 0 for empty buffers (e.g. arrays created from [] end up with Buffer#buffer.length = 0), but the spec currently claims pos_integer(). This will likely trigger Dialyzer warnings and is inconsistent with the implementation.

-spec size(Buffer :: arrow_buffer:buffer()) -> pos_integer().
size(Buffer) ->
    Len = Buffer#buffer.length,
    Len + arrow_utils:pad_len(Len).

[Benjamin-Philip]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)

src/arrow_buffer.erl:120

* `arrow_buffer:size/1` can legally return `0` for empty buffers (e.g. arrays created from `[]` end up with `Buffer#buffer.length = 0`), but the spec currently claims `pos_integer()`. This will likely trigger Dialyzer warnings and is inconsistent with the implementation.

-spec size(Buffer :: arrow_buffer:buffer()) -> pos_integer().
size(Buffer) ->
    Len = Buffer#buffer.length,
    Len + arrow_utils:pad_len(Len).

@kou, according to https://arrow.apache.org/docs/format/Columnar.html#buffer-alignment-and-padding, do empty buffers need to be padded? Right now, I'm taking 0 to be a multiple of 64, so I'm not padding. If we do need to pad 0-bit buffers, I'll have to fix that.

As of now, Copilot is right. pos_integer should be replaced with non_neg_integer since a size of 0 is possible.

[Benjamin-Philip]

For now I've fixed the typespec assuming no padding for 0-bit buffers, in the interest of time.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/arrow_buffer.erl:123

• size/1 doesn’t validate its input (unlike to_arrow/1 and to_erlang/1). Passing a non-#buffer{} will currently raise a low-level badrecord error instead of the module’s consistent badarg contract.

size(Buffer) ->
    Len = Buffer#buffer.length,
    Len + arrow_utils:pad_len(Len).

[Benjamin-Philip]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)

src/arrow_buffer.erl:123

* `size/1` doesn’t validate its input (unlike `to_arrow/1` and `to_erlang/1`). Passing a non-#buffer{} will currently raise a low-level `badrecord` error instead of the module’s consistent `badarg` contract.

size(Buffer) ->
    Len = Buffer#buffer.length,
    Len + arrow_utils:pad_len(Len).

This is fine. size/1 is an internal function that is not meant to be user facing. I don't want to waste CPU cycles on validations. Dialyzer will raise if inputs don't match the typespecs which is good enough here.

[kou]

+1

We don't need 64 bits padding for 0 bytes buffer.