Copilot commented on code in PR #50391:
URL: https://github.com/apache/arrow/pull/50391#discussion_r3532180209
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -160,7 +160,24 @@ def to_a
end
class PrimitiveArray < Array
- def initialize(type, size, validity_buffer, values_buffer)
+ include BufferAlignable
+
+ def initialize(*args)
+ n_args = args.size
+ if self.class.respond_to?(:type)
+ type = self.class.type
+ expected_n_args = "1 or 3"
+ else
+ type = args.shift
+ expected_n_args = "2 or 4"
+ end
+ args = build_data(args[0]) if args.size == 1
+ if args.size != 3
+ message =
+ "wrong number of arguments (given #{n_args}, #{expected_n_args})"
+ raise ArgumentError, message
+ end
+ size, validity_buffer, values_buffer = args
super(type, size, validity_buffer)
@values_buffer = values_buffer
end
Review Comment:
`PrimitiveArray#initialize` always treats a single argument as builder
input, which can produce confusing `NoMethodError`s (e.g.,
`Date32Array.new([..])` tries to call `pack_template`, and `Int8Array.new(10)`
would call `each_with_index` on an Integer). Also the raised `ArgumentError`
message is missing the word `expected`. Consider only invoking `build_data`
when the single argument is actually an Array, and fix the message formatting;
while here, set `@type` before calling `build_data` so the builder path can
reference it safely.
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -185,20 +202,35 @@ def each_buffer
def element_size
IO::Buffer.size_of(@type.buffer_type)
end
+
+ def build_data(data)
+ n = 0
+ validity_buffer_builder = nil
+ buffer = +"".b
+ pack_template = self.class.type.pack_template
+ data.each_with_index do |value, i|
Review Comment:
`PrimitiveArray#build_data` currently uses `self.class.type.pack_template`,
which breaks the explicit-type initializer path (`PrimitiveArray.new(type,
values)`) and also yields a hard-to-understand `NoMethodError` for primitive
arrays whose type doesn't implement `pack_template` (e.g., temporal types). Use
the resolved `@type` and raise a clear `ArgumentError` when `pack_template`
isn't supported.
##########
ruby/red-arrow-format/lib/arrow-format/type.rb:
##########
@@ -154,8 +162,12 @@ def buffer_type
:s16
end
- def build_array(size, validity_buffer, values_buffer)
- Int16Array.new(size, validity_buffer, values_buffer)
+ def pack_template
+ "s"
+ end
+
+ def build_array(...)
+ Int16Array.new(...)
end
Review Comment:
The new `pack_template` strings for multi-byte primitive types (e.g., `"s"`,
`"l"`, `"q"`, `"f"`, `"d"`) use native endianness. Arrow IPC buffers are
defined as little-endian, and other code here already packs little-endian
explicitly (e.g., `FileWriter` uses `pack("l<")`). To avoid producing incorrect
buffers on big-endian platforms (and to make the intent explicit), these
templates should use the little-endian variants (`s<`, `S<`, `l<`, `L<`, `q<`,
`Q<`, and `e`/`E` for float32/float64).
##########
ruby/red-arrow-format/test/test-primitive-array.rb:
##########
@@ -0,0 +1,94 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+class TestPrimitiveArray < Test::Unit::TestCase
+ def test_no_null
+ assert_equal([true, false],
+ ArrowFormat::BooleanArray.new([true, false]).to_a)
+ end
+
+ def test_null_multiple_bytes
+ values = [true] * 8 + [nil, false]
+ assert_equal(values,
+ ArrowFormat::BooleanArray.new(values).to_a)
+ end
+
+ def test_boolean
+ assert_equal([true, nil, false],
+ ArrowFormat::BooleanArray.new([true, nil, false]).to_a)
+ end
+
+ def test_int8
+ values = [-(2 ** 7), nil, (2 ** 7) - 1]
+ assert_equal(values,
+ ArrowFormat::Int8Array.new(values).to_a)
+ end
+
+ def test_uint8
+ values = [0, nil, (2 ** 8) - 1]
+ assert_equal(values,
+ ArrowFormat::UInt8Array.new(values).to_a)
+ end
+
+ def test_int16
+ values = [-(2 ** 15), nil, (2 ** 15) - 1]
+ assert_equal(values,
+ ArrowFormat::Int16Array.new(values).to_a)
+ end
+
+ def test_uint16
+ values = [0, nil, (2 ** 16) - 1]
+ assert_equal(values,
+ ArrowFormat::UInt16Array.new(values).to_a)
+ end
+
+ def test_int32
+ values = [-(2 ** 31), nil, (2 ** 31) - 1]
+ assert_equal(values,
+ ArrowFormat::Int32Array.new(values).to_a)
+ end
+
+ def test_uint32
+ values = [0, nil, (2 ** 32) - 1]
+ assert_equal(values,
+ ArrowFormat::UInt32Array.new(values).to_a)
+ end
+
+ def test_int64
+ values = [-(2 ** 63), nil, (2 ** 63) - 1]
+ assert_equal(values,
+ ArrowFormat::Int64Array.new(values).to_a)
+ end
+
+ def test_uint64
+ values = [0, nil, (2 ** 64) - 1]
+ assert_equal(values,
+ ArrowFormat::UInt64Array.new(values).to_a)
+ end
+
+ def test_float32
+ values = [-Float::INFINITY, -0.0, nil, +0.0, +Float::INFINITY]
+ assert_equal(values,
+ ArrowFormat::Float32Array.new(values).to_a)
+ end
+
+ def test_float64
+ values = [-Float::INFINITY, -0.0, nil, +0.0, +Float::INFINITY]
+ assert_equal(values,
+ ArrowFormat::Float64Array.new(values).to_a)
+ end
Review Comment:
The float tests include both `-0.0` and `+0.0`, but `assert_equal` uses `==`
and won't fail if the sign bit on zero is lost (because `-0.0 == 0.0`). Add an
assertion that distinguishes them (e.g., via reciprocals yielding `-Infinity`
vs `+Infinity`) so the new pack/unpack path is actually validated.
--
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]
