kou commented on code in PR #48462:
URL: https://github.com/apache/arrow/pull/48462#discussion_r2621503574
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -302,19 +285,47 @@ G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecuteContext,
garrow_execute_context, G_TYPE_
static_cast<GArrowExecuteContextPrivate *>(
\
garrow_execute_context_get_instance_private(GARROW_EXECUTE_CONTEXT(object)))
+enum {
+ PROP_EXECUTOR = 1,
+};
+
static void
garrow_execute_context_finalize(GObject *object)
{
auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
- priv->context.~ExecContext();
+ priv->context.reset();
Review Comment:
```suggestion
priv->context.~shared_ptr();
```
##########
c_glib/arrow-glib/executor.cpp:
##########
@@ -0,0 +1,176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <arrow-glib/error.hpp>
+#include <arrow-glib/executor.hpp>
+
+G_BEGIN_DECLS
+
+/**
+ * SECTION: executor
+ * @section_id: executor-classes
+ * @title: Executor classes
+ * @include: arrow-glib/arrow-glib.h
+ *
+ * #GArrowExecutor is the base class for executor implementations.
+ * #GArrowThreadPool is a class for thread pool management.
+ */
+
+typedef struct GArrowExecutorPrivate_
+{
+ arrow::internal::Executor *executor;
+} GArrowExecutorPrivate;
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecutor, garrow_executor, G_TYPE_OBJECT)
Review Comment:
```suggestion
G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE(GArrowExecutor, garrow_executor,
G_TYPE_OBJECT)
```
##########
c_glib/arrow-glib/compute.cpp:
##########
@@ -302,19 +285,47 @@ G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecuteContext,
garrow_execute_context, G_TYPE_
static_cast<GArrowExecuteContextPrivate *>(
\
garrow_execute_context_get_instance_private(GARROW_EXECUTE_CONTEXT(object)))
+enum {
+ PROP_EXECUTOR = 1,
+};
+
static void
garrow_execute_context_finalize(GObject *object)
{
auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
- priv->context.~ExecContext();
+ priv->context.reset();
G_OBJECT_CLASS(garrow_execute_context_parent_class)->finalize(object);
}
+static void
+garrow_execute_context_set_property(GObject *object,
+ guint prop_id,
+ const GValue *value,
+ GParamSpec *pspec)
+{
+ auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
+
+ switch (prop_id) {
+ case PROP_EXECUTOR:
+ {
+ auto executor = GARROW_EXECUTOR(g_value_get_object(value));
Review Comment:
Could you keep a reference to the given executor to prevent from GC?
```diff
diff --git a/c_glib/arrow-glib/compute.cpp b/c_glib/arrow-glib/compute.cpp
index 20f36c72e1..04002e30a4 100644
--- a/c_glib/arrow-glib/compute.cpp
+++ b/c_glib/arrow-glib/compute.cpp
@@ -274,10 +274,11 @@ garrow_compute_initialize(GError **error)
return garrow::check(error, status, "[compute][initialize]");
}
-typedef struct GArrowExecuteContextPrivate_
+struct GArrowExecuteContextPrivate
{
std::shared_ptr<arrow::compute::ExecContext> context;
-} GArrowExecuteContextPrivate;
+ GArrowExecutor *executor;
+};
G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecuteContext, garrow_execute_context,
G_TYPE_OBJECT)
@@ -289,11 +290,24 @@ enum {
PROP_EXECUTOR = 1,
};
+static void
+garrow_execute_context_dispose(GObject *object)
+{
+ auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
+
+ if (priv->executor) {
+ g_object_unref(priv->executor);
+ priv->executor = nullptr;
+ }
+
+ G_OBJECT_CLASS(garrow_execute_context_parent_class)->dispose(object);
+}
+
static void
garrow_execute_context_finalize(GObject *object)
{
auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
- priv->context.reset();
+ priv->context.~shared_ptr();
G_OBJECT_CLASS(garrow_execute_context_parent_class)->finalize(object);
}
@@ -308,8 +322,8 @@ garrow_execute_context_set_property(GObject *object,
switch (prop_id) {
case PROP_EXECUTOR:
{
- auto executor = GARROW_EXECUTOR(g_value_get_object(value));
- auto arrow_executor = garrow_executor_get_raw(executor);
+ priv->executor = GARROW_EXECUTOR(g_value_dup_object(value));
+ auto arrow_executor = garrow_executor_get_raw(priv->executor);
priv->context =
std::make_shared<arrow::compute::ExecContext>(arrow::default_memory_pool(),
arrow_executor);
@@ -321,11 +335,29 @@ garrow_execute_context_set_property(GObject *object,
}
}
+static void
+garrow_execute_context_get_property(GObject *object,
+ guint prop_id,
+ GValue *value,
+ GParamSpec *pspec)
+{
+ auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
+
+ switch (prop_id) {
+ case PROP_EXECUTOR:
+ g_value_set_object(value, priv->executor);
+ break;
+ default:
+ G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
+ break;
+ }
+}
+
static void
garrow_execute_context_init(GArrowExecuteContext *object)
{
auto priv = GARROW_EXECUTE_CONTEXT_GET_PRIVATE(object);
- priv->context = nullptr;
+ new (&priv->context) std::shared_ptr<arrow::compute::ExecContext>;
}
static void
@@ -333,16 +365,25 @@
garrow_execute_context_class_init(GArrowExecuteContextClass *klass)
{
auto gobject_class = G_OBJECT_CLASS(klass);
+ gobject_class->dispose = garrow_execute_context_dispose;
gobject_class->finalize = garrow_execute_context_finalize;
gobject_class->set_property = garrow_execute_context_set_property;
+ gobject_class->get_property = garrow_execute_context_get_property;
GParamSpec *spec;
+ /**
+ * GArrowExecuteContext:executor:
+ *
+ * The executor for execution.
+ *
+ * Since: 23.0.0
+ */
spec = g_param_spec_object(
"executor",
"Executor",
- "The GArrowExecutor for execution",
+ "The executor for execution",
GARROW_TYPE_EXECUTOR,
- static_cast<GParamFlags>(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));
+ static_cast<GParamFlags>(G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_EXECUTOR, spec);
}
diff --git a/c_glib/test/test-execute-context.rb
b/c_glib/test/test-execute-context.rb
new file mode 100644
index 0000000000..fc6873d0a3
--- /dev/null
+++ b/c_glib/test/test-execute-context.rb
@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+class TestExecuteContext < Test::Unit::TestCase
+ def setup
+ @context = Arrow::ExecuteContext.new
+ end
+
+ def test_executor
+ assert_nil(@context.executor)
+
+ executor = Arrow::ThreadPool.new(1)
+ context = Arrow::ExecuteContext.new(executor)
+ assert_equal(executor, context.executor)
+ end
+end
```
* We need to use `dispose` not `finalize` for GObject based children for
cycled reference. See also:
https://docs.gtk.org/gobject/tutorial.html#object-destruction
##########
c_glib/arrow-glib/executor.cpp:
##########
@@ -0,0 +1,176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <arrow-glib/error.hpp>
+#include <arrow-glib/executor.hpp>
+
+G_BEGIN_DECLS
+
+/**
+ * SECTION: executor
+ * @section_id: executor-classes
+ * @title: Executor classes
+ * @include: arrow-glib/arrow-glib.h
+ *
+ * #GArrowExecutor is the base class for executor implementations.
Review Comment:
```suggestion
* #GArrowExecutor is the base class for executor implementations.
*
```
##########
c_glib/arrow-glib/executor.cpp:
##########
@@ -0,0 +1,176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <arrow-glib/error.hpp>
+#include <arrow-glib/executor.hpp>
+
+G_BEGIN_DECLS
+
+/**
+ * SECTION: executor
+ * @section_id: executor-classes
+ * @title: Executor classes
+ * @include: arrow-glib/arrow-glib.h
+ *
+ * #GArrowExecutor is the base class for executor implementations.
+ * #GArrowThreadPool is a class for thread pool management.
+ */
+
+typedef struct GArrowExecutorPrivate_
+{
+ arrow::internal::Executor *executor;
+} GArrowExecutorPrivate;
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecutor, garrow_executor, G_TYPE_OBJECT)
+
+#define GARROW_EXECUTOR_GET_PRIVATE(obj)
\
+ static_cast<GArrowExecutorPrivate *>(
\
+ garrow_executor_get_instance_private(GARROW_EXECUTOR(obj)))
+
+static void
+garrow_executor_init(GArrowExecutor *object)
+{
+ auto priv = GARROW_EXECUTOR_GET_PRIVATE(object);
+ priv->executor = nullptr;
+}
+
+static void
+garrow_executor_class_init(GArrowExecutorClass *klass)
+{
+}
+
+typedef struct GArrowThreadPoolPrivate_
+{
+ std::shared_ptr<arrow::internal::ThreadPool> thread_pool;
+} GArrowThreadPoolPrivate;
+
+G_DEFINE_TYPE_WITH_PRIVATE(GArrowThreadPool, garrow_thread_pool,
GARROW_TYPE_EXECUTOR)
+
+#define GARROW_THREAD_POOL_GET_PRIVATE(obj)
\
+ static_cast<GArrowThreadPoolPrivate *>(
\
+ garrow_thread_pool_get_instance_private(GARROW_THREAD_POOL(obj)))
+
+enum {
+ PROP_THREAD_POOL = 1,
+};
+
+static void
+garrow_thread_pool_finalize(GObject *object)
+{
+ auto priv = GARROW_THREAD_POOL_GET_PRIVATE(object);
+ priv->thread_pool.~shared_ptr();
+ G_OBJECT_CLASS(garrow_thread_pool_parent_class)->finalize(object);
+}
+
+static void
+garrow_thread_pool_set_property(GObject *object,
+ guint prop_id,
+ const GValue *value,
+ GParamSpec *pspec)
+{
+ auto priv = GARROW_THREAD_POOL_GET_PRIVATE(object);
+ auto executor_priv = GARROW_EXECUTOR_GET_PRIVATE(GARROW_EXECUTOR(object));
+
+ switch (prop_id) {
+ case PROP_THREAD_POOL:
+ priv->thread_pool =
*static_cast<std::shared_ptr<arrow::internal::ThreadPool> *>(
+ g_value_get_pointer(value));
+ if (priv->thread_pool) {
+ executor_priv->executor = priv->thread_pool.get();
+ } else {
+ executor_priv->executor = nullptr;
+ }
+ break;
+ default:
+ G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
+ break;
+ }
+}
+
+static void
+garrow_thread_pool_init(GArrowThreadPool *object)
+{
+ auto priv = GARROW_THREAD_POOL_GET_PRIVATE(object);
+ new (&priv->thread_pool) std::shared_ptr<arrow::internal::ThreadPool>;
+ auto executor_priv = GARROW_EXECUTOR_GET_PRIVATE(GARROW_EXECUTOR(object));
+ executor_priv->executor = nullptr;
+}
Review Comment:
Could you move this to `GArrowExecutor`?
```diff
diff --git a/c_glib/arrow-glib/compute.cpp b/c_glib/arrow-glib/compute.cpp
index 20f36c72e1..af6a7c4998 100644
--- a/c_glib/arrow-glib/compute.cpp
+++ b/c_glib/arrow-glib/compute.cpp
@@ -312,7 +312,7 @@ garrow_execute_context_set_property(GObject *object,
auto arrow_executor = garrow_executor_get_raw(executor);
priv->context =
std::make_shared<arrow::compute::ExecContext>(arrow::default_memory_pool(),
- arrow_executor);
+ arrow_executor.get());
break;
}
default:
diff --git a/c_glib/arrow-glib/executor.cpp b/c_glib/arrow-glib/executor.cpp
index 34ed29b28b..fa2a64144e 100644
--- a/c_glib/arrow-glib/executor.cpp
+++ b/c_glib/arrow-glib/executor.cpp
@@ -20,6 +20,8 @@
#include <arrow-glib/error.hpp>
#include <arrow-glib/executor.hpp>
+#include <arrow/util/thread_pool.h>
+
G_BEGIN_DECLS
/**
@@ -32,10 +34,10 @@ G_BEGIN_DECLS
* #GArrowThreadPool is a class for thread pool management.
*/
-typedef struct GArrowExecutorPrivate_
+struct GArrowExecutorPrivate
{
- arrow::internal::Executor *executor;
-} GArrowExecutorPrivate;
+ std::shared_ptr<arrow::internal::Executor> executor;
+};
G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecutor, garrow_executor, G_TYPE_OBJECT)
@@ -43,59 +45,37 @@ G_DEFINE_TYPE_WITH_PRIVATE(GArrowExecutor,
garrow_executor, G_TYPE_OBJECT)
static_cast<GArrowExecutorPrivate *>(
\
garrow_executor_get_instance_private(GARROW_EXECUTOR(obj)))
+enum {
+ PROP_EXECUTOR = 1,
+};
+
static void
garrow_executor_init(GArrowExecutor *object)
{
auto priv = GARROW_EXECUTOR_GET_PRIVATE(object);
- priv->executor = nullptr;
+ new (&priv->executor) std::shared_ptr<arrow::internal::Executor>;
}
static void
-garrow_executor_class_init(GArrowExecutorClass *klass)
+garrow_executor_finalize(GObject *object)
{
-}
-
-typedef struct GArrowThreadPoolPrivate_
-{
- std::shared_ptr<arrow::internal::ThreadPool> thread_pool;
-} GArrowThreadPoolPrivate;
-
-G_DEFINE_TYPE_WITH_PRIVATE(GArrowThreadPool, garrow_thread_pool,
GARROW_TYPE_EXECUTOR)
-
-#define GARROW_THREAD_POOL_GET_PRIVATE(obj)
\
- static_cast<GArrowThreadPoolPrivate *>(
\
- garrow_thread_pool_get_instance_private(GARROW_THREAD_POOL(obj)))
-
-enum {
- PROP_THREAD_POOL = 1,
-};
-
-static void
-garrow_thread_pool_finalize(GObject *object)
-{
- auto priv = GARROW_THREAD_POOL_GET_PRIVATE(object);
- priv->thread_pool.~shared_ptr();
- G_OBJECT_CLASS(garrow_thread_pool_parent_class)->finalize(object);
+ auto priv = GARROW_EXECUTOR_GET_PRIVATE(object);
+ priv->executor.~shared_ptr();
+ G_OBJECT_CLASS(garrow_executor_parent_class)->finalize(object);
}
static void
-garrow_thread_pool_set_property(GObject *object,
- guint prop_id,
- const GValue *value,
- GParamSpec *pspec)
+garrow_executor_set_property(GObject *object,
+ guint prop_id,
+ const GValue *value,
+ GParamSpec *pspec)
{
- auto priv = GARROW_THREAD_POOL_GET_PRIVATE(object);
- auto executor_priv = GARROW_EXECUTOR_GET_PRIVATE(GARROW_EXECUTOR(object));
+ auto priv = GARROW_EXECUTOR_GET_PRIVATE(GARROW_EXECUTOR(object));
switch (prop_id) {
- case PROP_THREAD_POOL:
- priv->thread_pool =
*static_cast<std::shared_ptr<arrow::internal::ThreadPool> *>(
+ case PROP_EXECUTOR:
+ priv->executor =
*static_cast<std::shared_ptr<arrow::internal::Executor> *>(
g_value_get_pointer(value));
- if (priv->thread_pool) {
- executor_priv->executor = priv->thread_pool.get();
- } else {
- executor_priv->executor = nullptr;
- }
break;
default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
@@ -103,30 +83,33 @@ garrow_thread_pool_set_property(GObject *object,
}
}
+static void
+garrow_executor_class_init(GArrowExecutorClass *klass)
+{
+ auto gobject_class = G_OBJECT_CLASS(klass);
+
+ gobject_class->finalize = garrow_executor_finalize;
+ gobject_class->set_property = garrow_executor_set_property;
+
+ GParamSpec *spec;
+ spec = g_param_spec_pointer(
+ "executor",
+ "Executor",
+ "The raw std::shared_ptr<arrow::internal::Executor> *",
+ static_cast<GParamFlags>(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));
+ g_object_class_install_property(gobject_class, PROP_EXECUTOR, spec);
+}
+
+G_DEFINE_TYPE(GArrowThreadPool, garrow_thread_pool, GARROW_TYPE_EXECUTOR)
+
static void
garrow_thread_pool_init(GArrowThreadPool *object)
{
- auto priv = GARROW_THREAD_POOL_GET_PRIVATE(object);
- new (&priv->thread_pool) std::shared_ptr<arrow::internal::ThreadPool>;
- auto executor_priv = GARROW_EXECUTOR_GET_PRIVATE(GARROW_EXECUTOR(object));
- executor_priv->executor = nullptr;
}
static void
garrow_thread_pool_class_init(GArrowThreadPoolClass *klass)
{
- auto gobject_class = G_OBJECT_CLASS(klass);
-
- gobject_class->finalize = garrow_thread_pool_finalize;
- gobject_class->set_property = garrow_thread_pool_set_property;
-
- GParamSpec *spec;
- spec = g_param_spec_pointer(
- "thread-pool",
- "Thread pool",
- "The raw std::shared_ptr<arrow::internal::ThreadPool> *",
- static_cast<GParamFlags>(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));
- g_object_class_install_property(gobject_class, PROP_THREAD_POOL, spec);
}
/**
@@ -146,16 +129,16 @@ garrow_thread_pool_new(guint n_threads, GError **error)
if (garrow::check(error, arrow_thread_pool_result, "[thread-pool][new]"))
{
auto arrow_thread_pool = *arrow_thread_pool_result;
auto thread_pool = GARROW_THREAD_POOL(
- g_object_new(GARROW_TYPE_THREAD_POOL, "thread-pool",
&arrow_thread_pool, nullptr));
+ g_object_new(GARROW_TYPE_THREAD_POOL, "executor", &arrow_thread_pool,
nullptr));
return thread_pool;
} else {
- return NULL;
+ return nullptr;
}
}
G_END_DECLS
-arrow::internal::Executor *
+std::shared_ptr<arrow::internal::Executor>
garrow_executor_get_raw(GArrowExecutor *executor)
{
if (!executor)
@@ -164,13 +147,3 @@ garrow_executor_get_raw(GArrowExecutor *executor)
auto priv = GARROW_EXECUTOR_GET_PRIVATE(executor);
return priv->executor;
}
-
-std::shared_ptr<arrow::internal::ThreadPool>
-garrow_thread_pool_get_raw(GArrowThreadPool *thread_pool)
-{
- if (!thread_pool)
- return nullptr;
-
- auto priv = GARROW_THREAD_POOL_GET_PRIVATE(thread_pool);
- return priv->thread_pool;
-}
diff --git a/c_glib/arrow-glib/executor.hpp b/c_glib/arrow-glib/executor.hpp
index f2b8481d95..e91823186f 100644
--- a/c_glib/arrow-glib/executor.hpp
+++ b/c_glib/arrow-glib/executor.hpp
@@ -19,12 +19,7 @@
#pragma once
-#include <arrow/util/thread_pool.h>
-
#include <arrow-glib/executor.h>
-arrow::internal::Executor *
+std::shared_ptr<arrow::internal::Executor>
garrow_executor_get_raw(GArrowExecutor *executor);
-
-std::shared_ptr<arrow::internal::ThreadPool>
-garrow_thread_pool_get_raw(GArrowThreadPool *thread_pool);
```
* Our code uses `typedef XXXPrivate_ {...} XXXPrivate` but new code should
use `XXXPrivate {...}`. Old style is for C but we use C++. So new style is
better.
* We can use `std::shared_ptr` in `GArrowExecutor` and store
`std::shared_ptr<arrow::internal::ThreadPool>` to it because `ThreadPool` is a
subclass of `Executor`.
--
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]
