GH-50432: [Ruby] Add ArrowFormat::{Array,Bitmap}#==#50433
Merged
Conversation
|
|
github-actions
Bot
added
awaiting review
Contributor
There was a problem hiding this comment.
Pull request overview
Adds content-based equality and iteration support to the Ruby red-arrow-format ArrowFormat::Bitmap and ArrowFormat::Array hierarchy, and restructures tests to validate #== behavior across primitive types.
Changes:
- Add
ArrowFormat::Bitmap#==plus readers/aliases (offset,size,length) and adjust iteration to use@size. - Make
ArrowFormat::ArrayEnumerable, add/extend#eachimplementations forNullArray/PrimitiveArray/BooleanArray, and add#==implementations. - Replace the monolithic primitive array test with per-type test files, adding explicit
#==slice tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ruby/red-arrow-format/lib/arrow-format/bitmap.rb | Adds bitmap sizing APIs and content-based equality. |
| ruby/red-arrow-format/lib/arrow-format/array.rb | Adds Enumerable, #each, and #== for arrays and key subclasses. |
| ruby/red-arrow-format/test/test-bitmap.rb | Adds bitmap equality tests. |
| ruby/red-arrow-format/test/test-boolean-array.rb | Adds boolean array init and equality tests. |
| ruby/red-arrow-format/test/test-null-array.rb | Adds null array init and equality tests. |
| ruby/red-arrow-format/test/test-int8-array.rb | Adds int8 array init and equality tests. |
| ruby/red-arrow-format/test/test-int16-array.rb | Adds int16 array init and equality tests. |
| ruby/red-arrow-format/test/test-int32-array.rb | Adds int32 array init and equality tests. |
| ruby/red-arrow-format/test/test-int64-array.rb | Adds int64 array init and equality tests. |
| ruby/red-arrow-format/test/test-uint8-array.rb | Adds uint8 array init and equality tests. |
| ruby/red-arrow-format/test/test-uint16-array.rb | Adds uint16 array init and equality tests. |
| ruby/red-arrow-format/test/test-uint32-array.rb | Adds uint32 array init and equality tests. |
| ruby/red-arrow-format/test/test-uint64-array.rb | Adds uint64 array init and equality tests. |
| ruby/red-arrow-format/test/test-float32-array.rb | Adds float32 array init and equality tests. |
| ruby/red-arrow-format/test/test-float64-array.rb | Adds float64 array init and equality tests. |
| ruby/red-arrow-format/test/test-primitive-array.rb | Removes the prior consolidated primitive-array test file. |
Comments suppressed due to low confidence (1)
ruby/red-arrow-format/lib/arrow-format/array.rb:34
ArrowFormat::Arraynow includesEnumerable, but the class itself doesn’t define#each. Many subclasses (e.g.,VariableSizeBinaryArray,FixedSizeBinaryArray, list/struct/union, etc.) also don’t implement#each, somap,zip, and otherEnumerablemethods will raiseNoMethodErroron those arrays. Add a fallbackArray#eachimplementation (subclasses likeNullArray/PrimitiveArray/BooleanArraycan still override it for efficiency).
class Array
include Enumerable
attr_reader :type
attr_reader :size
alias_method :length, :size
attr_reader :offset
attr_reader :validity_buffer
def initialize(type, size, validity_buffer)
Comment on lines
+69
to
+78
| 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 |
| 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 |
Comment on lines
+248
to
+258
| 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 |
Comment on lines
+31
to
37
| 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 |
Comment on lines
65
to
+69
| def empty? | ||
| @size.zero? | ||
| end | ||
|
|
||
| def ==(other) |
Comment on lines
+247
to
+255
| 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 |
Comment on lines
+69
to
+73
| 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 |
Comment on lines
+247
to
+255
| 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 |
Comment on lines
+34
to
+36
| zip(other).all? do |bit, other_bit| | ||
| bit == other_bit | ||
| end |
Comment on lines
+69
to
+73
| 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 |
| assert_equal(array1, array2.slice(1, 3)) | ||
| end | ||
|
|
||
| def test_sliced_diffrent_content |
Comparing 2 bitmaps/arrays is a common operation. We should provide content based (not identity based) comparition feature.
Comment on lines
+69
to
+77
| 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 | ||
| return true | ||
| end | ||
| validity_bitmap == other.validity_bitmap | ||
| end |
Comment on lines
+69
to
+71
| def ==(other) | ||
| return false unless other.is_a?(self.class) | ||
| return false unless @size == other.size |
Member
Author
|
+1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Comparing 2 bitmaps/arrays is a common operation. We should provide content based (not identity based) comparition feature.
What changes are included in this PR?
ArrowFormat::Bitmap#==ArrowFormat::Bitmap#offsetArrowFormat::Bitmap#sizeArrowFormat::Bitmap#lengthArrowFormat::ArrayenumerableArrowFormat::Array#==ArrowFormat::NullArray#eachArrowFormat::PrimiveArray#eachArrowFormat::BooleanArray#eachAre these changes tested?
Yes.
Are there any user-facing changes?
Yes.
ArrowFormat::{Array,Bitmap}#==#50432