Copilot commented on code in PR #50433:
URL: https://github.com/apache/arrow/pull/50433#discussion_r3548937258
##########
ruby/red-arrow-format/lib/arrow-format/array.rb:
##########
@@ -64,18 +66,28 @@ def empty?
@size.zero?
end
+ def ==(other)
+ return false unless other.is_a?(self.class)
+ return false unless @size == other.size
+ return true if @validity_buffer.nil? and other.validity_buffer.nil?
+ if @offset == other.offset and @validity_buffer == other.validity_buffer
Review Comment:
`ArrowFormat::Array#==` currently only compares the validity buffer/bitmap
(and even returns true when both validity buffers are nil), so for
non-PrimitiveArray subclasses (e.g. VariableSizeBinaryArray, StructArray, etc.)
two arrays with different values but the same null layout can be considered
equal. Also, `ArrowFormat::Array` now includes `Enumerable` but doesn’t define
`#each`, so calling `each`/`map` on those subclasses will raise `NoMethodError`
unless they override `each`.
##########
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#==` uses `zip(other).all?`, which materializes an
intermediate array of pairs (potentially huge) before checking `all?`. If
`Array#==` is updated to compare element values, the current `super(other)`
call would also end up doing redundant work; this method can do its own
type/size check and then compare elements in a streaming way.
##########
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
Review Comment:
`Bitmap#==` uses `zip(other).all?`, which allocates an intermediate array of
pairs proportional to the bitmap length. This can cause significant memory
usage for large bitmaps; compare in a streaming loop instead.
--
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]
