Skip to content

Conversation

@stenlarsson
Copy link
Contributor

@stenlarsson stenlarsson commented Dec 5, 2025

Rationale for this change

The FixedSizeListArray class is not available in GLib/Ruby, and it can be the result of e.g. the extract_regex_span and list_slice compute functions.

What changes are included in this PR?

This adds the GArrowFixedSizeListArray and GArrowFixedSizeListArrayBuilder to GLib.

Are these changes tested?

Yes, with Ruby unit tests.

Are there any user-facing changes?

Yes, new classes.

@stenlarsson stenlarsson requested a review from kou as a code owner December 5, 2025 14:09
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

⚠️ GitHub issue #48362 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

def setup
require_gi_bindings(3, 1, 9)
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you define a helper method that gets the i-th value to shorten test code?

Suggested change
def get_value(array, i)
value = array.get_value(i)
value.length.times.collect do |j|
value.get_value(j)
end
end

Comment on lines 51 to 54
assert_equal([1, 2], array.get_value(0).length.times.collect {|i|
array.get_value(0).get_value(i)})
assert_equal([3, 4], array.get_value(1).length.times.collect {|i|
array.get_value(1).get_value(i)})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_equal([1, 2], array.get_value(0).length.times.collect {|i|
array.get_value(0).get_value(i)})
assert_equal([3, 4], array.get_value(1).length.times.collect {|i|
array.get_value(1).get_value(i)})
assert_equal([1, 2], get_value(array, 0))
assert_equal([3, 4], get_value(array, 1))

Comment on lines 78 to 79
assert_equal([1, 2], array.get_value(0).length.times.collect {|i|
array.get_value(0).get_value(i)})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_equal([1, 2], array.get_value(0).length.times.collect {|i|
array.get_value(0).get_value(i)})
assert_equal([1, 2], get_value(array, 0))

assert_equal(3, array.length)
assert_equal([1, 2], array.get_value(0).length.times.collect {|i|
array.get_value(0).get_value(i)})
assert_equal(true, array.null?(1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use assert with block for predicate? It shows more useful information on failure.

Suggested change
assert_equal(true, array.null?(1))
assert do
array.null?(1)
end

Comment on lines 81 to 82
assert_equal([5, 6], array.get_value(2).length.times.collect {|i|
array.get_value(2).get_value(i)})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert_equal([5, 6], array.get_value(2).length.times.collect {|i|
array.get_value(2).get_value(i)})
assert_equal([5, 6], get_value(array, 2))

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Dec 5, 2025
@stenlarsson stenlarsson force-pushed the glib-fixed-size-list-array branch from 8f7ca5f to 60aff82 Compare December 10, 2025 16:33
@stenlarsson
Copy link
Contributor Author

Thanks for the review @kou. Updated according to your suggestions.

@kou kou merged commit d5171c6 into apache:main Dec 10, 2025
12 checks passed
@kou kou removed the awaiting merge Awaiting merge label Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants