Copilot commented on code in PR #50433:
URL: https://github.com/apache/arrow/pull/50433#discussion_r3548676858
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -64,18 +66,29 @@ def empty?
@size.zero?
end
+ def ==(other)
+ return false unless other.is_a?(self.class)
+ return true if @validity_buffer.nil? and other.validity_buffer.nil?
+ if @offset == other.offset and
+ @size == other.size and
+ @validity_buffer == other.validity_buffer
+ return true
+ end
+ validity_bitmap == other.validity_bitmap
+ end
Review Comment:
`Array#==` currently only compares validity buffers/bitmaps (and can also
raise when exactly one side has `validity_buffer == nil` because it calls
`validity_bitmap` on `nil`). It also doesn’t compare `type` and can return
incorrect results for non-primitive arrays where values differ but validity
matches. Consider implementing content-based equality by comparing `type`,
`size`, and element values (via `#each`, with the `Array#each` fallback above).
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -173,7 +195,7 @@ def initialize(*args)
type = args.shift
expected_n_args = "2 or 4"
end
- args = build_data(args[0], type) if args.size == 1
+ args = build_data(args[0], type) if n_args == 1
Review Comment:
`PrimitiveArray#initialize` now uses `n_args == 1` to decide whether to call
`build_data`, but `n_args` is captured before shifting the `type` argument for
the generic `PrimitiveArray.new(type, data)` form. That breaks the
documented/encoded “2 or 4 args” path because after shifting, `args.size`
becomes 1 but `n_args` remains 2, so `build_data` is skipped and an
`ArgumentError` is raised.
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -192,15 +214,49 @@ def to_a
apply_validity(@values_buffer.values(@type.buffer_type, offset, @size))
end
+ def each(&block)
+ return to_enum(__method__) {@size} unless block_given?
+
+ each_value = Enumerator.new(@size) do |yielder|
+ offset = element_size * @offset
+ @values_buffer.each(@type.buffer_type, offset, @size) do |_, value|
+ yielder << value
+ end
+ end
+ if @validity_buffer.nil?
+ each_value.each(&block)
+ else
+ validity_bitmap.zip(each_value) do |is_valid, value|
+ if is_valid
+ yield(value)
+ else
+ yield(nil)
+ end
+ end
+ end
+ end
+
def each_buffer
- return to_enum(__method__) unless block_given?
+ return to_enum(__method__) {2} unless block_given?
yield(slice_bitmap_buffer(:validity, @validity_buffer))
yield(slice_fixed_element_size_buffer(:values,
@values_buffer,
element_size))
end
+ def ==(other)
+ return false unless super(other)
+ if @offset == other.offset and
+ @size == other.size and
+ @values_buffer == other.values_buffer
+ return true
+ end
+ zip(other).all? do |value, other_value|
+ value == other_value
+ end
+ end
Review Comment:
`PrimitiveArray#==` calls `super(other)` first. With a content-based
`Array#==`, that means primitive arrays will be fully compared once in
`Array#==` and then compared again here (and `zip(other)` allocates an
intermediate array). To keep `==` efficient, do the buffer/offset fast-path
first and then fall back to `super` for the element-wise comparison.
--
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]
