You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kou (via GitHub)" <gi...@apache.org> on 2026/05/12 21:45:57 UTC
[PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
kou opened a new pull request, #49969:
URL: https://github.com/apache/arrow/pull/49969
### Rationale for this change
Users can define any extension data type. So we may encounter an unknown extension type. We can't use `GArrowExtensionDataType` for an unknown extension data type because `GArrowExtensionDataType` is an abstract class.
### What changes are included in this PR?
This adds `GArrowUnknownExtensionDataType` and uses it for all unknown extension data types.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #49969:
URL: https://github.com/apache/arrow/pull/49969#issuecomment-4446722325
+1
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #49969:
URL: https://github.com/apache/arrow/pull/49969
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #49969:
URL: https://github.com/apache/arrow/pull/49969#discussion_r3238261538
##########
c_glib/arrow-glib/basic-data-type.cpp:
##########
@@ -2720,16 +2734,25 @@ garrow_data_type_new_raw(std::shared_ptr<arrow::DataType> *arrow_data_type)
type = GARROW_TYPE_DURATION_DATA_TYPE;
break;
case arrow::Type::type::EXTENSION:
+ type = GARROW_TYPE_UNKNOWN_EXTENSION_DATA_TYPE;
{
- auto g_extension_data_type =
- std::static_pointer_cast<garrow::GExtensionType>(*arrow_data_type);
- if (g_extension_data_type) {
- auto garrow_data_type = g_extension_data_type->garrow_data_type();
- g_object_ref(garrow_data_type);
- return GARROW_DATA_TYPE(garrow_data_type);
+ auto arrow_extension_data_type =
+ std::static_pointer_cast<arrow::ExtensionType>(*arrow_data_type);
+ auto name = arrow_extension_data_type->extension_name();
+ if (name == "arrow.fixed_shape_tensor") {
+ type = GARROW_TYPE_FIXED_SHAPE_TENSOR_DATA_TYPE;
+ } else if (name == "arrow.uuid") {
+ type = GARROW_TYPE_UUID_DATA_TYPE;
+ } else {
+ auto g_extension_data_type =
+ std::dynamic_pointer_cast<garrow::GExtensionType>(*arrow_data_type);
+ if (g_extension_data_type) {
+ auto garrow_data_type = g_extension_data_type->garrow_data_type();
+ g_object_ref(garrow_data_type);
+ return GARROW_DATA_TYPE(garrow_data_type);
+ }
}
Review Comment:
In the EXTENSION case, the code checks canonical extension_name values (fixed_shape_tensor/uuid) before attempting to unwrap garrow::GExtensionType. If a GLib-defined custom extension type happens to use one of these reserved names, this will incorrectly construct a GArrowFixedShapeTensorDataType/GArrowUUIDDataType wrapper around a garrow::GExtensionType instance, and later static_pointer_casts inside those wrappers (e.g., to arrow::extension::FixedShapeTensorType) can become UB/crash. Prefer unwrapping garrow::GExtensionType first (dynamic_pointer_cast and return its garrow_data_type), then fall back to name-based mapping for non-GLib extension types, and finally to GArrowUnknownExtensionDataType.
##########
c_glib/arrow-glib/basic-data-type.cpp:
##########
@@ -2720,16 +2734,25 @@ garrow_data_type_new_raw(std::shared_ptr<arrow::DataType> *arrow_data_type)
type = GARROW_TYPE_DURATION_DATA_TYPE;
break;
case arrow::Type::type::EXTENSION:
+ type = GARROW_TYPE_UNKNOWN_EXTENSION_DATA_TYPE;
{
- auto g_extension_data_type =
- std::static_pointer_cast<garrow::GExtensionType>(*arrow_data_type);
- if (g_extension_data_type) {
- auto garrow_data_type = g_extension_data_type->garrow_data_type();
- g_object_ref(garrow_data_type);
- return GARROW_DATA_TYPE(garrow_data_type);
+ auto arrow_extension_data_type =
+ std::static_pointer_cast<arrow::ExtensionType>(*arrow_data_type);
+ auto name = arrow_extension_data_type->extension_name();
+ if (name == "arrow.fixed_shape_tensor") {
+ type = GARROW_TYPE_FIXED_SHAPE_TENSOR_DATA_TYPE;
+ } else if (name == "arrow.uuid") {
+ type = GARROW_TYPE_UUID_DATA_TYPE;
+ } else {
Review Comment:
There doesn’t appear to be a regression test covering the newly added fallback path for truly unknown extension types (e.g., arrow::extension::OpaqueType / "arrow.opaque"), which is the main crash scenario described in GH-49956. Consider adding a test that round-trips a schema/field containing an unknown extension type through the GLib conversion and asserts it doesn’t segfault and produces a GArrowUnknownExtensionDataType wrapper.
##########
c_glib/arrow-glib/basic-data-type.cpp:
##########
@@ -2080,6 +2080,20 @@ namespace garrow {
G_BEGIN_DECLS
+G_DEFINE_TYPE(GArrowUnknownExtensionDataType,
+ garrow_unknown_extension_data_type,
+ GARROW_TYPE_EXTENSION_DATA_TYPE)
+
+static void
+garrow_unknown_extension_data_type_init(GArrowUnknownExtensionDataType *object)
+{
+}
+
+static void
+garrow_unknown_extension_data_type_class_init(GArrowUnknownExtensionDataTypeClass *klass)
+{
+}
Review Comment:
GArrowUnknownExtensionDataType is a new public GObject type, but it isn’t mentioned in the gtk-doc SECTION: basic-data-type overview list at the top of this file. Please add it there (and any other relevant public API docs) so generated reference docs reflect the new user-facing type.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #49969:
URL: https://github.com/apache/arrow/pull/49969#issuecomment-4463741837
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit c1036681b099c5f9b0684a710be04bb7619e926f.
There was 1 benchmark result indicating a performance regression:
- Commit Run on `test-mac-arm` at [2026-05-14 22:01:16Z](https://conbench.arrow-dev.org/compare/runs/1501df35c080476b9d63f87d4f7deaf7...ccf9fff936b0424c8bf28ca55b8b226e/)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-17, scale_factor=1](https://conbench.arrow-dev.org/compare/benchmarks/06a071993b5b73ee8000456df87e5d0e...06a0647a3a697db28000402895e669bb)
The [full Conbench report](https://github.com/apache/arrow/runs/76261562255) has more details. It also includes information about 141 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #49969:
URL: https://github.com/apache/arrow/pull/49969#issuecomment-4435092226
:warning: GitHub issue #49956 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-49956: [GLib] Add fallback data type for unknown extension data type [arrow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #49969:
URL: https://github.com/apache/arrow/pull/49969#discussion_r3230063484
##########
c_glib/arrow-glib/basic-data-type.cpp:
##########
@@ -2720,16 +2734,25 @@ garrow_data_type_new_raw(std::shared_ptr<arrow::DataType> *arrow_data_type)
type = GARROW_TYPE_DURATION_DATA_TYPE;
break;
case arrow::Type::type::EXTENSION:
+ type = GARROW_TYPE_UNKNOWN_EXTENSION_DATA_TYPE;
{
- auto g_extension_data_type =
- std::static_pointer_cast<garrow::GExtensionType>(*arrow_data_type);
- if (g_extension_data_type) {
- auto garrow_data_type = g_extension_data_type->garrow_data_type();
- g_object_ref(garrow_data_type);
- return GARROW_DATA_TYPE(garrow_data_type);
+ auto arrow_extension_data_type =
+ std::dynamic_pointer_cast<arrow::ExtensionType>(*arrow_data_type);
Review Comment:
In the EXTENSION case, `*arrow_data_type` is already known (via `id() == arrow::Type::EXTENSION`) to be an `arrow::ExtensionType`, so `std::dynamic_pointer_cast<arrow::ExtensionType>` is redundant and introduces RTTI overhead plus a potential nullptr dereference if assumptions ever change. Prefer `std::static_pointer_cast<arrow::ExtensionType>` here (and keep the `dynamic_pointer_cast` only for the `garrow::GExtensionType` check).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org