GH-50832: [Ruby] Add ArrowFormat::FixedSizeBinaryArray.new(byte_widt…h, values) - #50854
GH-50832: [Ruby] Add ArrowFormat::FixedSizeBinaryArray.new(byte_widt…h, values)#50854otegami wants to merge 1 commit into
ArrowFormat::FixedSizeBinaryArray.new(byte_widt…h, values)#50854Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the Ruby red-arrow-format bindings to make it easier to construct ArrowFormat::FixedSizeBinaryArray instances from Ruby objects by accepting either a byte width shorthand (Integer) or a FixedSizeBinaryType, while keeping the existing 4-argument constructor form.
Changes:
- Extend
ArrowFormat::FixedSizeBinaryArray.newto accept(byte_width, values)and(type, values)shorthands. - Add
ArrowFormat::FixedSizeBinaryType.try_convertto support the byte-width shorthand conversion pattern used by other temporal types. - Add a dedicated test suite for
FixedSizeBinaryArrayinitialization and equality behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ruby/red-arrow-format/test/test-fixed-size-binary-array.rb | Adds test coverage for the new FixedSizeBinaryArray construction forms and basic behavior. |
| ruby/red-arrow-format/lib/arrow-format/type.rb | Adds FixedSizeBinaryType.try_convert to support shorthand conversion from Integer byte widths. |
| ruby/red-arrow-format/lib/arrow-format/array.rb | Implements the new FixedSizeBinaryArray initializer paths and builds buffers from Ruby values. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| byte_width = @type.byte_width | ||
| values = 0.step(@size * byte_width - 1, byte_width).collect do |offset| | ||
| @values_buffer.get_string(offset, byte_width) | ||
| end |
There was a problem hiding this comment.
This is an existing problem. This PR doesn't change #to_a.
#[] and #each are missing too. I'll open a follow-up issue for them.
| class FixedSizeBinaryArray < Array | ||
| def initialize(type, size, validity_buffer, values_buffer) | ||
| include BufferAlignable | ||
|
|
||
| def initialize(type, *args) | ||
| unless type.is_a?(Type) |
There was a problem hiding this comment.
ArrowFormat::Array#== compares only the type, the size, and the validity structure. ArrowFormat::BinaryArray and ArrowFormat::UTF8Array behave the same, so this isn't specific to fixed-size binary.
Comparing values needs #each, which is missing. I'll handle it in the same follow-up issue.
| values.append_as_bytes(null_value) | ||
| else | ||
| unless value.bytesize == byte_width | ||
| message = "value size must be #{byte_width}: #{value.bytesize}" |
There was a problem hiding this comment.
Could you show the actual value for easy to debug?
| message = "value size must be #{byte_width}: #{value.bytesize}" | |
| message = "value size must be #{byte_width}: #{value.inspect}" |
There was a problem hiding this comment.
fix: https://github.com/apache/arrow/pull/50854/changes#diff-94226bf2a6f57e453bae1425fc7d92410f15de7effc0bc83fefe8e2d3e87ca29R909 Sure. Thank you for reviewing.
github-actions
Bot
added
awaiting changes
…e_width, values)` ### Rationale for this change Building a fixed size binary Arrow array from Ruby objects is convenient. ### What changes are included in this PR? Accept: * `ArrowFormat::FixedSizeBinaryArray.new(byte_width, values)` * `ArrowFormat::FixedSizeBinaryArray.new(type, values)` `ArrowFormat::FixedSizeBinaryType.try_convert` is added for the byte width shorthand, in the same way as `ArrowFormat::Time32Array.new(unit, values)`. The existing 4 arguments form is kept. Decimal arrays are out of scope. `ArrowFormat::DecimalArray#to_a` returns `BigDecimal`, so building one from Ruby objects needs a conversion between `BigDecimal` and two's complement. I would like to open a separate issue for it. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * GitHub Issue: apache#50832
5e20123 to
cf6b3d1
Compare
github-actions
Bot
added
awaiting change review
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
ruby/red-arrow-format/lib/arrow-format/array.rb:860
FixedSizeBinaryArray#initializeconverts any non-Typefirst argument viaFixedSizeBinaryType.try_convert(...) || type. This affects subclasses likeDecimalArray:DecimalArray.new(16, values)will build aDecimalArraywith aFixedSizeBinaryType(not aDecimalType) and then crash inDecimalArray#to_awhen it expects@type.precision/@type.scale. Also, if conversion fails (e.g., a String), leavingtypeunchanged will cause a laterNoMethodErrorontype.byte_width. Restrict the shorthand conversion toFixedSizeBinaryArrayitself and raise a clearArgumentErrorwhen the type cannot be converted.
unless type.is_a?(Type)
type = FixedSizeBinaryType.try_convert(type) || type
end
Rationale for this change
Building a fixed size binary Arrow array from Ruby objects is convenient.
What changes are included in this PR?
Accept:
ArrowFormat::FixedSizeBinaryArray.new(byte_width, values)ArrowFormat::FixedSizeBinaryArray.new(type, values)ArrowFormat::FixedSizeBinaryType.try_convertis added for the byte width shorthand, in the same way asArrowFormat::Time32Array.new(unit, values). The existing 4 arguments form is kept.Decimal arrays are out of scope.
ArrowFormat::DecimalArray#to_areturnsBigDecimal, so building one from Ruby objects needs a conversion betweenBigDecimaland two's complement. I would like to open a separate issue for it.Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.
ArrowFormat::FixedSizeBinaryArray.new(byte_width, values)#50832