Copilot commented on code in PR #50433:
URL: https://github.com/apache/arrow/pull/50433#discussion_r3548913535
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -64,18 +66,28 @@ def empty?
@size.zero?
end
+ def ==(other)
Review Comment:
`Array#==` currently only compares validity bitmaps (and uses `and` in a
modifier form), which can both produce false positives for non-primitive arrays
(e.g., `VariableSizeBinaryArray` values are ignored) and can raise if one side
has `@validity_buffer.nil?` while the other has a non-nil validity buffer
(because `validity_bitmap` is built from `nil`). To make equality content-based
across all `ArrowFormat::Array` subclasses, compare enumerated values and
provide a default `each` implementation in the base class.
##########
ruby/red-arrow-format/lib/arrow-format/bitmap.rb:
##########
@@ -19,10 +19,21 @@ module ArrowFormat
class Bitmap
include Enumerable
- def initialize(buffer, offset, n_values)
+ attr_reader :offset
+ attr_reader :size
+ alias_method :length, :size
+ def initialize(buffer, offset, size)
@buffer = buffer
@offset = offset
- @n_values = n_values
+ @size = size
+ end
+
+ def ==(other)
+ return false unless other.is_a?(self.class)
+ return false unless @size == other.size
+ zip(other).all? do |bit, other_bit|
+ bit == other_bit
+ end
end
Review Comment:
`Bitmap#==` uses `zip(other).all?`, which builds an intermediate array of
pairs. You can avoid the allocation by using `zip` with a block and
early-returning on mismatch.
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -192,15 +213,47 @@ 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 @values_buffer == other.values_buffer
+ return true
+ end
+ zip(other).all? do |value, other_value|
+ value == other_value
+ end
+ end
Review Comment:
`PrimitiveArray#==` currently uses `zip(other).all?`, which allocates an
intermediate array of pairs for the whole array. You can keep the
buffer-identity fast path, then delegate to `Array#==` (which will use the
optimized `PrimitiveArray#each`) to avoid the allocation.
--
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]
