Copilot commented on code in PR #48989:
URL: https://github.com/apache/arrow/pull/48989#discussion_r2731495286
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -1558,9 +1689,28 @@ garrow_aggregation_new(const gchar *function,
NULL));
}
-G_DEFINE_TYPE(GArrowAggregateNodeOptions,
- garrow_aggregate_node_options,
- GARROW_TYPE_EXECUTE_NODE_OPTIONS)
+struct GArrowAggregateNodeOptionsPrivate
+{
+ GList *aggregations;
+};
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowAggregateNodeOptions,
+ garrow_aggregate_node_options,
+ GARROW_TYPE_EXECUTE_NODE_OPTIONS)
+
+#define GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(object)
\
+ static_cast<GArrowAggregateNodeOptionsPrivate *>(
\
+ garrow_aggregate_node_options_get_instance_private(
\
+ GARROW_AGGREGATE_NODE_OPTIONS(object)))
+
+static void
+garrow_aggregate_node_options_dispose(GObject *object)
+{
+ auto priv = GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(object);
+ g_list_free_full(priv->aggregations, g_object_unref);
+ priv->aggregations = nullptr;
+ G_OBJECT_CLASS(garrow_aggregate_node_options_parent_class)->dispose(object);
+}
Review Comment:
`GArrowAggregateNodeOptionsPrivate::aggregations` is freed in
`garrow_aggregate_node_options_dispose()` but never initialized in
`garrow_aggregate_node_options_init()`, which can cause `g_list_free_full()` to
operate on garbage if the field isn't explicitly set. As with other GObject
classes in this file, this member should be initialized to `NULL` in the init
function before being used here.
##########
c_glib/arrow-glib/expression.hpp:
##########
@@ -27,6 +27,18 @@ GARROW_EXTERN
GArrowExpression *
garrow_expression_new_raw(const arrow::compute::Expression &arrow_expression);
+GARROW_EXTERN
+GArrowExpression *
+garrow_expression_new_raw(const arrow::compute::Expression &arrow_expression,
+ const gchar *first_property_name,
+ ...);
+
+GARROW_EXTERN
+GArrowExpression *
+garrow_expression_new_raw_valist(const arrow::compute::Expression
&arrow_expression,
+ const gchar *first_property_name,
+ va_list args);
Review Comment:
This header declares functions taking a `va_list`
(`garrow_expression_new_raw_valist`) but does not include
`<stdarg.h>`/`<cstdarg>` itself. To avoid relying on transitive includes and
potential build issues for consumers that include this header directly,
consider explicitly including the appropriate stdarg header here.
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -1301,14 +1385,39 @@ garrow_filter_node_options_new(GArrowExpression
*expression)
{
auto arrow_expression = garrow_expression_get_raw(expression);
auto arrow_options = new arrow::acero::FilterNodeOptions(*arrow_expression);
- auto options =
- g_object_new(GARROW_TYPE_FILTER_NODE_OPTIONS, "options", arrow_options,
NULL);
+ auto options = g_object_new(GARROW_TYPE_FILTER_NODE_OPTIONS,
+ "options",
+ arrow_options,
+ "expression",
+ expression,
+ nullptr);
return GARROW_FILTER_NODE_OPTIONS(options);
}
-G_DEFINE_TYPE(GArrowProjectNodeOptions,
- garrow_project_node_options,
- GARROW_TYPE_EXECUTE_NODE_OPTIONS)
+struct GArrowProjectNodeOptionsPrivate
+{
+ GList *expressions;
+};
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowProjectNodeOptions,
+ garrow_project_node_options,
+ GARROW_TYPE_EXECUTE_NODE_OPTIONS)
+
+#define GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(object)
\
+ static_cast<GArrowProjectNodeOptionsPrivate *>(
\
+ garrow_project_node_options_get_instance_private(
\
+ GARROW_PROJECT_NODE_OPTIONS(object)))
+
+static void
+garrow_project_node_options_dispose(GObject *object)
+{
+ auto priv = GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(object);
+
+ g_list_free_full(priv->expressions, g_object_unref);
+ priv->expressions = nullptr;
+
+ G_OBJECT_CLASS(garrow_project_node_options_parent_class)->dispose(object);
Review Comment:
`GArrowProjectNodeOptionsPrivate::expressions` is freed in
`garrow_project_node_options_dispose()` but is never initialized in
`garrow_project_node_options_init()`, so `g_list_free_full()` may receive an
indeterminate pointer if the field was not set. Please initialize `expressions`
to `NULL` in the init function to make dispose safe for partially-constructed
or default instances.
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -1623,10 +1776,29 @@ garrow_aggregate_node_options_new(GList *aggregations,
auto arrow_options = new
arrow::acero::AggregateNodeOptions(std::move(arrow_aggregates),
std::move(arrow_keys));
auto options =
- g_object_new(GARROW_TYPE_AGGREGATE_NODE_OPTIONS, "options", arrow_options,
NULL);
+ g_object_new(GARROW_TYPE_AGGREGATE_NODE_OPTIONS, "options", arrow_options,
nullptr);
+ auto priv = GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(options);
+ priv->aggregations =
+ g_list_copy_deep(aggregations, reinterpret_cast<GCopyFunc>(g_object_ref),
nullptr);
return GARROW_AGGREGATE_NODE_OPTIONS(options);
}
+/**
+ * garrow_aggregate_node_options_get_aggregations:
+ * @options: A #GArrowAggregateNodeOptions.
+ *
+ * Returns: (transfer none) (element-type GArrowAggregation): Aggregations
+ * of this options.
Review Comment:
The docstring sentence "Aggregations of this options." is ungrammatical; it
should be "Aggregations of these options." or similar. Please adjust the
wording to improve the clarity of the API documentation.
```suggestion
* of these options.
```
##########
c_glib/arrow-glib/expression.cpp:
##########
@@ -112,7 +144,71 @@ garrow_expression_equal(GArrowExpression *expression,
GArrowExpression *other_ex
return priv->expression.Equals(other_priv->expression);
}
-G_DEFINE_TYPE(GArrowLiteralExpression, garrow_literal_expression,
GARROW_TYPE_EXPRESSION)
+enum {
+ PROP_DATUM = 1,
+};
+
+struct GArrowLiteralExpressionPrivate
+{
+ GArrowDatum *datum;
+};
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowLiteralExpression,
+ garrow_literal_expression,
+ GARROW_TYPE_EXPRESSION)
+
+#define GARROW_LITERAL_EXPRESSION_GET_PRIVATE(object)
\
+ static_cast<GArrowLiteralExpressionPrivate *>(
\
+
garrow_literal_expression_get_instance_private(GARROW_LITERAL_EXPRESSION(object)))
+
+static void
+garrow_literal_expression_dispose(GObject *object)
+{
+ auto priv = GARROW_LITERAL_EXPRESSION_GET_PRIVATE(object);
+
+ if (priv->datum) {
+ g_object_unref(priv->datum);
+ priv->datum = nullptr;
+ }
+
+ G_OBJECT_CLASS(garrow_literal_expression_parent_class)->dispose(object);
+}
Review Comment:
`GArrowLiteralExpressionPrivate::datum` is accessed and unref'd in
`garrow_literal_expression_dispose()` but never initialized in
`garrow_literal_expression_init()`, which can lead to undefined behavior if an
instance is created without setting the `datum` property. Please initialize
`datum` to `NULL` in `garrow_literal_expression_init()` before it is used in
dispose/get_property.
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -1354,9 +1466,28 @@ garrow_project_node_options_new(GList *expressions,
gchar **names, gsize n_names
new arrow::acero::ProjectNodeOptions(arrow_expressions, arrow_names);
auto options =
g_object_new(GARROW_TYPE_PROJECT_NODE_OPTIONS, "options", arrow_options,
NULL);
+ auto priv = GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(options);
+ priv->expressions =
+ g_list_copy_deep(expressions, reinterpret_cast<GCopyFunc>(g_object_ref),
nullptr);
return GARROW_PROJECT_NODE_OPTIONS(options);
}
+/**
+ * garrow_project_node_options_get_expressions:
+ * @options: A #GArrowProjectNodeOptions.
+ *
+ * Returns: (transfer none) (element-type GArrowExpression): Expressions
+ * of this options.
Review Comment:
The docstring sentence "Expressions of this options." is ungrammatical; it
should be "Expressions of these options." or similar. Updating the wording will
make the API documentation clearer for users.
```suggestion
* of these options.
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -1274,9 +1274,71 @@ garrow_source_node_options_new_table(GArrowTable *table)
return options;
}
-G_DEFINE_TYPE(GArrowFilterNodeOptions,
- garrow_filter_node_options,
- GARROW_TYPE_EXECUTE_NODE_OPTIONS)
+enum {
+ PROP_EXPRESSION = 1,
+};
+
+struct GArrowFilterNodeOptionsPrivate
+{
+ GArrowExpression *expression;
+};
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowFilterNodeOptions,
+ garrow_filter_node_options,
+ GARROW_TYPE_EXECUTE_NODE_OPTIONS)
+
+#define GARROW_FILTER_NODE_OPTIONS_GET_PRIVATE(object)
\
+ static_cast<GArrowFilterNodeOptionsPrivate *>(
\
+
garrow_filter_node_options_get_instance_private(GARROW_FILTER_NODE_OPTIONS(object)))
+
+static void
+garrow_filter_node_options_dispose(GObject *object)
+{
+ auto priv = GARROW_FILTER_NODE_OPTIONS_GET_PRIVATE(object);
+
+ if (priv->expression) {
+ g_object_unref(priv->expression);
+ priv->expression = nullptr;
+ }
+
+ G_OBJECT_CLASS(garrow_filter_node_options_parent_class)->dispose(object);
+}
Review Comment:
`GArrowFilterNodeOptionsPrivate::expression` is used in
`garrow_filter_node_options_dispose()` and
`garrow_filter_node_options_get_property()` but is never initialized in
`garrow_filter_node_options_init()`. To avoid undefined behavior when options
are constructed without the `"expression"` property (or if construction fails
early), initialize `expression` to `NULL` in the init function before it is
accessed here.
--
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]
