Copilot commented on code in PR #104:
URL: https://github.com/apache/arrow-erlang/pull/104#discussion_r3579315505
##########
src/arrow_array.erl:
##########
@@ -148,42 +116,42 @@ from_erlang(Layout, Value, Opts) ->
%% Array Data and Metadata Access %%
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
-%% @doc Returns the layout of an array.
+-doc "Returns the layout of an array.".
-spec layout(Array :: #array{}) -> Layout :: layout().
layout(Array) ->
Array#array.layout.
-%% @doc Returns the type of an array.
+-doc "Returns the type of an array.".
-spec type(Array :: #array{}) -> Type :: arrow_type:arrow_type().
type(Array) ->
Array#array.type.
-%% @doc Returns the length of an array.
+-doc "Returns the length of an array.".
-spec len(Array :: #array{}) -> Length :: pos_integer().
len(Array) ->
Array#array.len.
-%% @doc Returns the length of an array.
+-doc "Returns the length of an array.".
Review Comment:
`element_len/1`’s doc string currently repeats `len/1` (“Returns the length
of an array.”). Since `element_len` is the per-element length, the doc should
say that explicitly.
##########
src/arrow_array.erl:
##########
@@ -15,87 +15,51 @@
% specific language governing permissions and limitations
% under the License.
-%% @doc Provides a record, a behaviour, and functions to work with Apache Arrow
-%% Arrays.
-%%
-%% This module as mentioned, provides conveniences for working with Apache
Arrow
-%% Arrays[1] of different Layouts[2]. Firstly, it provides a record to
represent
-%% all the Layout. Secondly, it provides a behaviour for different layouts to
-%% adhere to common functionality. Lastly, it provides functions to work with
-%% Arrays in a common manner.
-%%
-%% == The Structure of an Array ==
-%%
-%% `arrow''s implementation of an Array has the following fields in its
-%% record definition:
-%%
-%% <ol>
-%% <li>
-%% `layout', of type {@link atom()}, which represents the Layout of the
-%% Array.
-%% </li>
-%% <li>
-%% `type', of type {@link arrow_type:arrow_type()}, which represents
-%% the Logical Type[3] of the Array.
-%% </li>
-%% <li>`len', of type {@link pos_integer()}, which represents the Array's
Length[4].</li>
-%% <li>
-%% `element_len', of type {@link pos_integer()} or `undefined', which
-%% represents the Length of each element in an Array. Currently it only
has
-%% an integer value in the Fixed-Size List Layout[5]
-%% </li>
-%% <li>
-%% `null_count', of type {@link non_neg_integer()}, which represents the
-%% Array's Null Count[4], or the number of undefined values in the Array.
-%% </li>
-%% <li>
-%% `validity_bitmap', which is a buffer (`arrow_buffer') or the atom
-%% `undefined', which represents the Array's Validity Bitmap[6].
-%% </li>
-%% <li>
-%% `offsets', which is a buffer (`arrow_buffer') which represents the
-%% Offsets[7], or the start position of each slot in the data buffer of an
-%% Array.
-%% </li>
-%% <li>
-%% `data', which is a buffer (`arrow_buffer'), which represents the
-%% Array's Value Buffer, whose layout differs based on the Array Layout.
-%% </li>
-%% </ol>
-%%
-%% Certain fields are not required for certain layouts. For example, for the
-%% Fixed-Sized Primitive Layout, the offsets field is not required, in which
-%% case it is assigned as `undefined'. Similarly, the `validity_bitmap' is not
-%% required if there are no null values, in which case it is also assigned as
-%% `undefined'.
-%%
-%% == The Behaviour of an Array ==
-%%
-%% As of right now, a layout needs to implement the `c:from_erlang/2'
callback. This is
-%% then used in the `from_erlang/3' function to create new arrays.
-%%
-%% == Functions for working with Arrays ==
-%%
-%% As of right now, only functions to access the various fields, to create
-%% new arrays, and to serialize arrays to arrow exist.
-%%
-%% == References ==
-%%
-%% [1]: [https://arrow.apache.org/docs/format/Glossary.html#term-array]
-%%
-%% [2]:
[https://arrow.apache.org/docs/format/Glossary.html#term-physical-layout]
-%%
-%% [3]: [https://arrow.apache.org/docs/format/Glossary.html#term-type]
-%%
-%% [4]: [https://arrow.apache.org/docs/format/Columnar.html#null-count]
-%%
-%% [5]:
[https://arrow.apache.org/docs/format/Columnar.html#fixed-size-list-layout]
-%%
-%% [6]: [https://arrow.apache.org/docs/format/Columnar.html#validity-bitmaps]
-%%
-%% [7]:
[https://arrow.apache.org/docs/format/Columnar.html#variable-size-binary-layout]
-%% @end
-module(arrow_array).
+-moduledoc """
+Provides a record, a behaviour, and functions to work with Apache Arrow Arrays.
+
+This module as mentioned, provides conveniences for working with Apache Arrow
+[Arrays](https://arrow.apache.org/docs/format/Glossary.html#term-array) of
+different
+[Layouts](https://arrow.apache.org/docs/format/Glossary.html#term-physical-layout).
+Firstly, it provides a record to represent all the Layout. Secondly, it
provides
+a behaviour for different layouts to adhere to common functionality. Lastly, it
+provides functions to work with Arrays in a common manner.
+
+# The Structure of an Array
+
+`arrow`'s implementation of an Array has the following fields in its record
+definition, each corresponding to a field in the Arrow Columnar Format
+specification:
+
+| Field | Type | Representing
|
+|-------------------|-----------------------------|-------------------------------------------------------------------------------------------|
+| `layout` | `t:atom/0` | Layout
|
+| `type` | `t:arrow_type:arrow_type/0` | [Logical
Type](https://arrow.apache.org/docs/format/Glossary.html#term-type)
|
+| `len` | `t:pos_integer/0` |
[Length](https://arrow.apache.org/docs/format/Columnar.html#array-lengths)
|
+| `element_len` | `t:pos_integer/0` | Length of each element
|
+| `null_count` | `t:non_neg_integer/0` | [Null
Count](https://arrow.apache.org/docs/format/Columnar.html#null-count)
|
+| `validity_bitmap` | `arrow_buffer` | [Validity
Bitmap](https://arrow.apache.org/docs/format/Columnar.html#validity-bitmaps)
|
+| `offsets` | `arrow_buffer` |
[Offsets](https://arrow.apache.org/docs/format/Columnar.html#variable-size-binary-layout)
|
+| `data` | `arrow_buffer` | Value Buffer
|
Review Comment:
The moduledoc table’s type column doesn’t match the actual `#array{}` record
fields (see `src/arrow_array.hrl`): `element_len` can be `undefined`,
`validity_bitmap`/`offsets` are `#buffer{} | undefined`, and `data` can be a
nested `#array{}`. This makes the rendered docs misleading.
##########
src/arrow_array.erl:
##########
@@ -192,24 +160,24 @@ data(Array) ->
%% Array Serialization %%
%%%%%%%%%%%%%%%%%%%%%%%%%
-%% @doc Serializes an array into the Arrow binary form.
-%%
-%% Serializes the buffers of an Array and concatenates them in the following
-%% order:
-%%
-%% <ol>
-%% <li>`validity'</li>
-%% <li>`offsets'</li>
-%% <li>`data'</li>
-%% </ol>
-%%
-%% In case an array doesn't have any of the following buffers, it is ommitted.
-%% (e.g. validity in arrays with a null count of 0, offsets in fixed primitive
-%% arrays). In the case of a nested array, `data' will be serialized form of
-%% nested array.
-%%
-%% Do note that this is just binary form that includes the buffers in an Array,
-%% and not IPC.
+-doc """
+Serializes an array into the Arrow binary form.
+
+Serializes the buffers of an Array and concatenates them in the following
+order:
+
+1. `validity`
+2. `offsets`
+3. `data`
+
+In case an array doesn't have any of the following buffers, it is ommitted.
+(e.g. validity in arrays with a null count of 0, offsets in fixed primitive
+arrays). In the case of a nested array, `data` will be serialized form of
nested
+array.
+
+Do note that this is just binary form that includes the buffers in an Array,
+and not IPC.
+""".
Review Comment:
This doc block has a typo (“ommitted”) and a few grammar issues that will
show up in the generated Markdown output. Also, there’s a trailing space after
the opening `"""`.
##########
.github/workflows/erlang.yml:
##########
@@ -81,8 +81,8 @@ jobs:
- name: Install Erlang/OTP
uses: erlef/setup-beam@54075bcc5e249e4758d363f27d099f55d843f124 #
v1.24.1
with:
- otp-version: 25.1.0
- rebar3-version: '3.18.0'
+ otp-version: 29.0
+ rebar3-version: 3.27.0
Review Comment:
Same YAML version-coercion risk here: unquoted `29.0` can be treated as a
number and lose the `.0` when converted to a string for action inputs.
##########
.github/workflows/erlang.yml:
##########
@@ -43,8 +43,8 @@ jobs:
- name: Install Erlang/OTP
uses: erlef/setup-beam@54075bcc5e249e4758d363f27d099f55d843f124 #
v1.24.1
with:
- otp-version: 25.1.0
- rebar3-version: '3.18.0'
+ otp-version: 29.0
+ rebar3-version: 3.27.0
Review Comment:
`otp-version: 29.0` will be parsed by YAML as a number (unlike the old
`25.1.0`, which is a string), and may get stringified as `"29"` when passed to
the action. Quoting avoids accidental version coercion and keeps behavior
consistent.
##########
.github/workflows/erlang.yml:
##########
@@ -122,8 +122,8 @@ jobs:
- name: Install Erlang/OTP
uses: erlef/setup-beam@54075bcc5e249e4758d363f27d099f55d843f124 #
v1.24.1
with:
- otp-version: 25.1.0
- rebar3-version: '3.18.0'
+ otp-version: 29.0
+ rebar3-version: 3.27.0
Review Comment:
Same YAML version-coercion risk here: unquoted `29.0` can be treated as a
number and lose the `.0` when converted to a string for action inputs.
##########
.github/workflows/docs.yml:
##########
@@ -51,8 +51,8 @@ jobs:
- name: Install Erlang/OTP
uses: erlef/setup-beam@54075bcc5e249e4758d363f27d099f55d843f124 #
v1.24.1
with:
- otp-version: 25.1.0
- rebar3-version: '3.18.0'
+ otp-version: 29.0
+ rebar3-version: 3.27.0
Review Comment:
Same YAML version-coercion risk here: unquoted `29.0` can be treated as a
number and lose the `.0` when converted to a string for action inputs.
--
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]
