GH-48488: [GLib][Ruby] Add GArrowListSliceOptions#48489
Conversation
|
|
|
The property |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
+1
The property
return_fixed_size_listhas the typestd::optional<bool>. While it would make sense in Ruby, I couldn't figure out how to set a boolean to null in GLib, so I created an enum instead.
OK. Let's wrap it in ruby/red-arrow/ to accept options.return_fixed_size_list = true/= false/= nil as a follow-up task.
c_glib/arrow-glib/compute.cpp
Outdated
| if (options->stop.has_value()) { | ||
| g_value_set_int64(value, options->stop.value()); | ||
| } else { | ||
| g_value_set_int64(value, -1); // Sentinel value for "not set" |
There was a problem hiding this comment.
Could you add #define GARROW_LIST_SLICE_OPTIONS_STOP_UNSPECIFIED -1 or something to compute.h and use it here and others?
gobject-introspection gem defines it in Ruby automatically. So we can use it in our tests.
c_glib/arrow-glib/compute.cpp
Outdated
| GArrowListSliceOptions * | ||
| garrow_list_slice_options_new(void) | ||
| { | ||
| auto options = g_object_new(GARROW_TYPE_LIST_SLICE_OPTIONS, NULL); |
There was a problem hiding this comment.
Could you use nullptr instead of NULL because C++ prefers nullptr?
| auto options = g_object_new(GARROW_TYPE_LIST_SLICE_OPTIONS, NULL); | |
| auto options = g_object_new(GARROW_TYPE_LIST_SLICE_OPTIONS, nullptr); |
stenlarsson
force-pushed
the
glib-list-slice-options
branch
from
166f463 to
081867a
Compare
|
Good idea! I added Ruby wrappers for both the Unfortunately the tests fails due to #48610. |
kou
force-pushed
the
glib-list-slice-options
branch
from
081867a to
db2c03b
Compare
github-actions
bot
added
awaiting changes
| # list element’s length, nil values will be appended to create the requested | ||
| # slice size. The default of nil will return the same type which was passed in. | ||
| # | ||
| # Since: 23.0.0 |
There was a problem hiding this comment.
YARD uses @since 23.0.0 not Since: 23.0.0 syntax: https://www.rubydoc.info/docs/yard/file/docs/Tags.md#since
This also overrides the stop getter and setters in Ruby so that nil can be used instead.
To allow setting true/false/nil.
stenlarsson
force-pushed
the
glib-list-slice-options
branch
from
f94166d to
6789198
Compare
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 3dc9821. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
2 participants
Rationale for this change
The
ListSliceOptionsclass is not available in GLib/Ruby, and it is used together with thelist_slicecompute function.What changes are included in this PR?
This adds the
ListSliceOptionsclass to GLib.Are these changes tested?
Yes, with Ruby unit tests.
Are there any user-facing changes?
Yes, a new class.