kou commented on code in PR #46926:
URL: https://github.com/apache/arrow/pull/46926#discussion_r2723076814
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -4334,8 +4402,8 @@ garrow_rank_options_set_property(GObject *object,
switch (prop_id) {
case PROP_RANK_OPTIONS_NULL_PLACEMENT:
- options->null_placement =
- static_cast<arrow::compute::NullPlacement>(g_value_get_enum(value));
+ options->null_placement = garrow_optional_null_placement_to_raw(
+ static_cast<GArrowOptionalNullPlacement>(g_value_get_enum(value)));
Review Comment:
Could you move them after the `G_END_DECLS`
https://github.com/apache/arrow/blob/75ef03165dc7e54aca0f842f54598833ddbd0a01/c_glib/arrow-glib/compute.cpp#L10666
? We place exported C++ functions after `G_END_DECLS`.
##########
c_glib/arrow-glib/compute.h:
##########
@@ -509,6 +509,28 @@ typedef enum /*<prefix=GARROW_NULL_PLACEMENT_>*/ {
GARROW_NULL_PLACEMENT_AT_END,
} GArrowNullPlacement;
+/**
+ * GArrowOptionalNullPlacement:
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_AT_START:
+ * Place nulls and NaNs before any non-null values.
+ * NaNs will come after nulls.
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_AT_END:
+ * Place nulls and NaNs after any non-null values.
+ * NaNs will come before nulls.
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED:
+ * Do not specify null placement.
+ * Null placement should instead
Review Comment:
Is this garbage?
```suggestion
```
##########
c_glib/arrow-glib/compute.h:
##########
@@ -509,6 +509,28 @@ typedef enum /*<prefix=GARROW_NULL_PLACEMENT_>*/ {
GARROW_NULL_PLACEMENT_AT_END,
} GArrowNullPlacement;
+/**
+ * GArrowOptionalNullPlacement:
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_AT_START:
+ * Place nulls and NaNs before any non-null values.
+ * NaNs will come after nulls.
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_AT_END:
+ * Place nulls and NaNs after any non-null values.
+ * NaNs will come before nulls.
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED:
+ * Do not specify null placement.
+ * Null placement should instead
+ *
+ * They are corresponding to `std::optional<arrow::compute::NullPlacement>`
values.
+ *
+ * Since: 24.0.0
+ */
+typedef enum /*<prefix=GARROW_OPTIONAL_NULL_PLACEMENT_>*/ {
+ GARROW_OPTIONAL_NULL_PLACEMENT_AT_START,
+ GARROW_OPTIONAL_NULL_PLACEMENT_AT_END,
+ GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED,
Review Comment:
How about using `-1` that will be never used in
`arrow::compute::NullPlacement`?
```suggestion
GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED = -1,
GARROW_OPTIONAL_NULL_PLACEMENT_AT_START,
GARROW_OPTIONAL_NULL_PLACEMENT_AT_END,
```
##########
c_glib/arrow-glib/compute.h:
##########
@@ -509,6 +509,28 @@ typedef enum /*<prefix=GARROW_NULL_PLACEMENT_>*/ {
GARROW_NULL_PLACEMENT_AT_END,
} GArrowNullPlacement;
+/**
+ * GArrowOptionalNullPlacement:
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_AT_START:
+ * Place nulls and NaNs before any non-null values.
+ * NaNs will come after nulls.
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_AT_END:
+ * Place nulls and NaNs after any non-null values.
+ * NaNs will come before nulls.
+ * @GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED:
+ * Do not specify null placement.
+ * Null placement should instead
+ *
+ * They are corresponding to `std::optional<arrow::compute::NullPlacement>`
values.
Review Comment:
```suggestion
* They are corresponding to `arrow::compute::NullPlacement` values except
* `GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED`.
* `GARROW_OPTIONAL_NULL_PLACEMENT_UNSPECIFIED` is used to specify
* `std::nullopt`.
```
--
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]
