kou commented on code in PR #38584:
URL: https://github.com/apache/arrow/pull/38584#discussion_r1382684566
##########
cpp/src/arrow/compute/api_vector.h:
##########
@@ -177,21 +174,17 @@ class ARROW_EXPORT RankOptions : public FunctionOptions {
};
explicit RankOptions(std::vector<SortKey> sort_keys = {},
- NullPlacement null_placement = NullPlacement::AtEnd,
Tiebreaker tiebreaker = RankOptions::First);
/// Convenience constructor for array inputs
explicit RankOptions(SortOrder order,
- NullPlacement null_placement = NullPlacement::AtEnd,
Review Comment:
I think that we should keep this signature to specify `NullPlacement`.
##########
cpp/src/arrow/engine/substrait/serde_test.cc:
##########
@@ -5378,8 +5378,8 @@ TEST(Substrait, SortAndFetch) {
}
TEST(Substrait, MixedSort) {
- // Substrait allows two sort keys with differing direction but Acero
- // does not. We should detect this and reject it.
+ // Substrait allows two sort keys with differing direction and
+ // acero is now supported.
Review Comment:
Can we remove this comment?
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3117,6 +3135,7 @@ garrow_sort_key_class_init(GArrowSortKeyClass *klass)
GArrowSortKey *
garrow_sort_key_new(const gchar *target,
GArrowSortOrder order,
+ GArrowNullPlacement null_placement,
Review Comment:
Could you add `@null_placement: Whether nulls and NaNs are placed at the
start or at the end.` to the docstring?
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3127,6 +3146,7 @@ garrow_sort_key_new(const gchar *target,
}
auto sort_key = g_object_new(GARROW_TYPE_SORT_KEY,
"order", order,
+ "null_placement", null_placement,
Review Comment:
```suggestion
"null-placement", null_placement,
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3103,6 +3112,15 @@ garrow_sort_key_class_init(GArrowSortKeyClass *klass)
static_cast<GParamFlags>(G_PARAM_READWRITE |
G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_SORT_KEY_ORDER, spec);
+
+ spec = g_param_spec_enum("null_placement",
Review Comment:
```suggestion
/**
* GArrowSortKey::null-placement:
*
* Whether nulls and NaNs are placed at the start or at the end.
*
* Since: 15.0.0
*/
spec = g_param_spec_enum("null_placement",
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3103,6 +3112,15 @@ garrow_sort_key_class_init(GArrowSortKeyClass *klass)
static_cast<GParamFlags>(G_PARAM_READWRITE |
G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_SORT_KEY_ORDER, spec);
+
+ spec = g_param_spec_enum("null_placement",
+ "NullPlacement",
Review Comment:
```suggestion
spec = g_param_spec_enum("null-placement",
"Null placement",
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -4461,7 +4481,6 @@ garrow_index_options_new(void)
enum {
- PROP_RANK_OPTIONS_NULL_PLACEMENT = 1,
PROP_RANK_OPTIONS_TIEBREAKER,
Review Comment:
```suggestion
PROP_RANK_OPTIONS_TIEBREAKER = 1,
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3019,6 +3020,10 @@ garrow_sort_key_set_property(GObject *object,
priv->sort_key.order =
static_cast<arrow::compute::SortOrder>(g_value_get_enum(value));
break;
+ case PROP_SORT_KEY_NULL_PLACEMENT:
+ priv->sort_key.null_placement =
+ static_cast<arrow::compute::NullPlacement>(g_value_get_enum(value));
Review Comment:
```suggestion
static_cast<arrow::compute::NullPlacement>(g_value_get_enum(value));
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -3103,6 +3112,15 @@ garrow_sort_key_class_init(GArrowSortKeyClass *klass)
static_cast<GParamFlags>(G_PARAM_READWRITE |
G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_SORT_KEY_ORDER, spec);
+
+ spec = g_param_spec_enum("null_placement",
+ "NullPlacement",
+ "How to sort nulls",
Review Comment:
```suggestion
"Whether nulls and NaNs are placed at the start
or at the end",
```
--
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]
