From ed6e951d53bd99966def64a16ded6db1a51ca2e7 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Wed, 21 Oct 2026 09:43:04 +0900 Subject: [PATCH] GH-54687: [GLib] Add missing owner references to GBytes --- c_glib/arrow-glib/basic-array.cpp | 27 ++- c_glib/arrow-glib/buffer.cpp | 19 ++- c_glib/parquet-glib/metadata.cpp | 2 +- c_glib/parquet-glib/statistics.cpp | 174 ++++++++------------ c_glib/parquet-glib/statistics.hpp | 4 +- c_glib/test/test-binary-array.rb | 15 +- c_glib/test/test-binary-view-array.rb | 13 ++ c_glib/test/test-fixed-size-binary-array.rb | 16 +- c_glib/test/test-resizable-buffer.rb | 20 +++ c_glib/test/test-string-view-array.rb | 13 ++ ci/scripts/c_glib_test.sh | 11 +- 11 files changed, 182 insertions(+), 132 deletions(-) diff --git a/c_glib/arrow-glib/basic-array.cpp b/c_glib/arrow-glib/basic-array.cpp index a60c589788fe..4b4584f1e59d 100644 --- a/c_glib/arrow-glib/basic-array.cpp +++ b/c_glib/arrow-glib/basic-array.cpp @@ -2329,7 +2329,10 @@ garrow_base_binary_array_get_value(GArrowArray *array, gint64 i) auto arrow_array = garrow_array_get_raw(array); auto arrow_binary_array = std::static_pointer_cast(arrow_array); auto view = arrow_binary_array->GetView(i); - return g_bytes_new_static(view.data(), view.length()); + return g_bytes_new_with_free_func(view.data(), + view.length(), + g_object_unref, + g_object_ref(array)); }; template @@ -2763,7 +2766,10 @@ garrow_binary_view_array_get_value(GArrowBinaryViewArray *array, gint64 i) { auto arrow_array = garrow_array_get_raw(GARROW_ARRAY(array)); auto view = static_cast(arrow_array.get())->GetView(i); - return g_bytes_new_static(view.data(), view.length()); + return g_bytes_new_with_free_func(view.data(), + view.length(), + g_object_unref, + g_object_ref(array)); } G_DEFINE_TYPE(GArrowStringViewArray, @@ -2834,7 +2840,10 @@ garrow_string_view_array_get_value(GArrowStringViewArray *array, gint64 i) { auto arrow_array = garrow_array_get_raw(GARROW_ARRAY(array)); auto view = static_cast(arrow_array.get())->GetView(i); - return g_bytes_new_static(view.data(), view.length()); + return g_bytes_new_with_free_func(view.data(), + view.length(), + g_object_unref, + g_object_ref(array)); } G_DEFINE_TYPE(GArrowDate32Array, garrow_date32_array, GARROW_TYPE_NUMERIC_ARRAY) @@ -3617,7 +3626,10 @@ garrow_fixed_size_binary_array_get_value(GArrowFixedSizeBinaryArray *array, gint auto arrow_binary_array = std::static_pointer_cast(arrow_array); auto value = arrow_binary_array->GetValue(i); - return g_bytes_new_static(value, arrow_binary_array->byte_width()); + return g_bytes_new_with_free_func(value, + arrow_binary_array->byte_width(), + g_object_unref, + g_object_ref(array)); } /** @@ -3635,8 +3647,11 @@ garrow_fixed_size_binary_array_get_values_bytes(GArrowFixedSizeBinaryArray *arra auto arrow_binary_array = std::static_pointer_cast(arrow_array); auto value = arrow_binary_array->raw_values(); - return g_bytes_new_static(value, - arrow_binary_array->byte_width() * arrow_array->length()); + return g_bytes_new_with_free_func(value, + arrow_binary_array->byte_width() * + arrow_array->length(), + g_object_unref, + g_object_ref(array)); } G_DEFINE_TYPE(GArrowDecimal32Array, diff --git a/c_glib/arrow-glib/buffer.cpp b/c_glib/arrow-glib/buffer.cpp index c5ca418f63f0..8681ab4efa6e 100644 --- a/c_glib/arrow-glib/buffer.cpp +++ b/c_glib/arrow-glib/buffer.cpp @@ -281,8 +281,9 @@ garrow_buffer_get_capacity(GArrowBuffer *buffer) * garrow_buffer_get_data: * @buffer: A #GArrowBuffer. * - * Returns: (transfer full): The data of the buffer. The data is owned by - * the buffer. You should not free or modify the data. + * Returns: (transfer full): The data of the buffer. The data isn't + * copied. The returned #GBytes keeps the data alive. You should not + * modify the data. * * Since: 0.3.0 */ @@ -296,7 +297,10 @@ garrow_buffer_get_data(GArrowBuffer *buffer) } auto arrow_buffer = garrow_buffer_get_raw(buffer); - auto data = g_bytes_new_static(arrow_buffer->data(), arrow_buffer->size()); + auto data = g_bytes_new_with_free_func(arrow_buffer->data(), + arrow_buffer->size(), + g_object_unref, + g_object_ref(buffer)); return data; } @@ -305,8 +309,8 @@ garrow_buffer_get_data(GArrowBuffer *buffer) * @buffer: A #GArrowBuffer. * * Returns: (transfer full) (nullable): The data of the buffer. If the - * buffer is immutable, it returns %NULL. The data is owned by the - * buffer. You should not free the data. + * buffer is immutable, it returns %NULL. The data isn't copied. The + * returned #GBytes keeps the data alive. * * Since: 0.3.0 */ @@ -324,7 +328,10 @@ garrow_buffer_get_mutable_data(GArrowBuffer *buffer) return priv->data; } - return g_bytes_new_static(arrow_buffer->mutable_data(), arrow_buffer->size()); + return g_bytes_new_with_free_func(arrow_buffer->mutable_data(), + arrow_buffer->size(), + g_object_unref, + g_object_ref(buffer)); } /** diff --git a/c_glib/parquet-glib/metadata.cpp b/c_glib/parquet-glib/metadata.cpp index 9e18decb3fd6..3f5da0cfa4a2 100644 --- a/c_glib/parquet-glib/metadata.cpp +++ b/c_glib/parquet-glib/metadata.cpp @@ -217,7 +217,7 @@ gparquet_column_chunk_metadata_get_statistics(GParquetColumnChunkMetadata *metad auto parquet_metadata = gparquet_column_chunk_metadata_get_raw(metadata); auto parquet_statistics = parquet_metadata->statistics(); if (parquet_statistics) { - return gparquet_statistics_new_raw(&parquet_statistics); + return gparquet_statistics_new_raw(&parquet_statistics, metadata); } else { return NULL; } diff --git a/c_glib/parquet-glib/statistics.cpp b/c_glib/parquet-glib/statistics.cpp index 9f0074e70e71..172c7d041200 100644 --- a/c_glib/parquet-glib/statistics.cpp +++ b/c_glib/parquet-glib/statistics.cpp @@ -19,6 +19,7 @@ #include +#include #include #include @@ -54,10 +55,12 @@ G_BEGIN_DECLS struct GParquetStatisticsPrivate { std::shared_ptr statistics; + GParquetColumnChunkMetadata *metadata; }; enum { PROP_STATISTICS = 1, + PROP_METADATA, }; G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE(GParquetStatistics, @@ -68,6 +71,17 @@ G_DEFINE_ABSTRACT_TYPE_WITH_PRIVATE(GParquetStatistics, static_cast( \ gparquet_statistics_get_instance_private(GPARQUET_STATISTICS(object))) +static void +gparquet_statistics_dispose(GObject *object) +{ + auto priv = GPARQUET_STATISTICS_GET_PRIVATE(object); + if (priv->metadata) { + g_object_unref(priv->metadata); + priv->metadata = nullptr; + } + G_OBJECT_CLASS(gparquet_statistics_parent_class)->dispose(object); +} + static void gparquet_statistics_finalize(GObject *object) { @@ -89,6 +103,9 @@ gparquet_statistics_set_property(GObject *object, priv->statistics = *static_cast *>(g_value_get_pointer(value)); break; + case PROP_METADATA: + priv->metadata = GPARQUET_COLUMN_CHUNK_METADATA(g_value_dup_object(value)); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -106,6 +123,7 @@ static void gparquet_statistics_class_init(GParquetStatisticsClass *klass) { auto gobject_class = G_OBJECT_CLASS(klass); + gobject_class->dispose = gparquet_statistics_dispose; gobject_class->finalize = gparquet_statistics_finalize; gobject_class->set_property = gparquet_statistics_set_property; @@ -116,6 +134,14 @@ gparquet_statistics_class_init(GParquetStatisticsClass *klass) "The raw std::shared_ptr", static_cast(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)); g_object_class_install_property(gobject_class, PROP_STATISTICS, spec); + + spec = g_param_spec_object( + "metadata", + "Metadata", + "The column chunk metadata of this statistics", + GPARQUET_TYPE_COLUMN_CHUNK_METADATA, + static_cast(G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)); + g_object_class_install_property(gobject_class, PROP_METADATA, spec); } /** @@ -467,38 +493,9 @@ gparquet_double_statistics_get_max(GParquetDoubleStatistics *statistics) return gparquet_typed_statistics_get_max(statistics); } -struct GParquetByteArrayStatisticsPrivate -{ - GBytes *min; - GBytes *max; -}; - -G_DEFINE_TYPE_WITH_PRIVATE(GParquetByteArrayStatistics, - gparquet_byte_array_statistics, - GPARQUET_TYPE_STATISTICS) - -#define GPARQUET_BYTE_ARRAY_STATISTICS_GET_PRIVATE(object) \ - static_cast( \ - gparquet_byte_array_statistics_get_instance_private( \ - GPARQUET_BYTE_ARRAY_STATISTICS(object))) - -static void -gparquet_byte_array_statistics_dispose(GObject *object) -{ - auto priv = GPARQUET_BYTE_ARRAY_STATISTICS_GET_PRIVATE(object); - - if (priv->min) { - g_bytes_unref(priv->min); - priv->min = nullptr; - } - - if (priv->max) { - g_bytes_unref(priv->max); - priv->max = nullptr; - } - - G_OBJECT_CLASS(gparquet_byte_array_statistics_parent_class)->dispose(object); -} +G_DEFINE_TYPE(GParquetByteArrayStatistics, + gparquet_byte_array_statistics, + GPARQUET_TYPE_STATISTICS) static void gparquet_byte_array_statistics_init(GParquetByteArrayStatistics *object) @@ -508,83 +505,51 @@ gparquet_byte_array_statistics_init(GParquetByteArrayStatistics *object) static void gparquet_byte_array_statistics_class_init(GParquetByteArrayStatisticsClass *klass) { - auto gobject_class = G_OBJECT_CLASS(klass); - gobject_class->dispose = gparquet_byte_array_statistics_dispose; } /** * gparquet_byte_array_statistics_get_min: * @statistics: A #GParquetByteArrayStatistics. * - * Returns: (transfer none): The minimum value. + * Returns: (transfer full): The minimum value. The value isn't + * copied. The returned #GBytes keeps the value alive. * * Since: 8.0.0 */ GBytes * gparquet_byte_array_statistics_get_min(GParquetByteArrayStatistics *statistics) { - auto priv = GPARQUET_BYTE_ARRAY_STATISTICS_GET_PRIVATE(statistics); - if (!priv->min) { - const auto &parquet_min = - gparquet_typed_statistics_get_min(statistics); - priv->min = g_bytes_new_static(parquet_min.ptr, parquet_min.len); - } - return priv->min; + const auto &parquet_min = + gparquet_typed_statistics_get_min(statistics); + return g_bytes_new_with_free_func(parquet_min.ptr, + parquet_min.len, + g_object_unref, + g_object_ref(statistics)); } /** * gparquet_byte_array_statistics_get_max: * @statistics: A #GParquetByteArrayStatistics. * - * Returns: (transfer none): The maximum value. + * Returns: (transfer full): The maximum value. The value isn't + * copied. The returned #GBytes keeps the value alive. * * Since: 8.0.0 */ GBytes * gparquet_byte_array_statistics_get_max(GParquetByteArrayStatistics *statistics) { - auto priv = GPARQUET_BYTE_ARRAY_STATISTICS_GET_PRIVATE(statistics); - if (!priv->max) { - const auto &parquet_max = - gparquet_typed_statistics_get_max(statistics); - priv->max = g_bytes_new_static(parquet_max.ptr, parquet_max.len); - } - return priv->max; + const auto &parquet_max = + gparquet_typed_statistics_get_max(statistics); + return g_bytes_new_with_free_func(parquet_max.ptr, + parquet_max.len, + g_object_unref, + g_object_ref(statistics)); } -struct GParquetFixedLengthByteArrayStatisticsPrivate -{ - GBytes *min; - GBytes *max; -}; - -G_DEFINE_TYPE_WITH_PRIVATE(GParquetFixedLengthByteArrayStatistics, - gparquet_fixed_length_byte_array_statistics, - GPARQUET_TYPE_STATISTICS) - -#define GPARQUET_FIXED_LENGTH_BYTE_ARRAY_STATISTICS_GET_PRIVATE(object) \ - static_cast( \ - gparquet_fixed_length_byte_array_statistics_get_instance_private( \ - GPARQUET_FIXED_LENGTH_BYTE_ARRAY_STATISTICS(object))) - -static void -gparquet_fixed_length_byte_array_statistics_dispose(GObject *object) -{ - auto priv = GPARQUET_FIXED_LENGTH_BYTE_ARRAY_STATISTICS_GET_PRIVATE(object); - - if (priv->min) { - g_bytes_unref(priv->min); - priv->min = nullptr; - } - - if (priv->max) { - g_bytes_unref(priv->max); - priv->max = nullptr; - } - - G_OBJECT_CLASS(gparquet_fixed_length_byte_array_statistics_parent_class) - ->dispose(object); -} +G_DEFINE_TYPE(GParquetFixedLengthByteArrayStatistics, + gparquet_fixed_length_byte_array_statistics, + GPARQUET_TYPE_STATISTICS) static void gparquet_fixed_length_byte_array_statistics_init( @@ -596,15 +561,14 @@ static void gparquet_fixed_length_byte_array_statistics_class_init( GParquetFixedLengthByteArrayStatisticsClass *klass) { - auto gobject_class = G_OBJECT_CLASS(klass); - gobject_class->dispose = gparquet_fixed_length_byte_array_statistics_dispose; } /** * gparquet_fixed_length_byte_array_statistics_get_min: * @statistics: A #GParquetFixedLengthByteArrayStatistics. * - * Returns: (transfer none): The minimum value. + * Returns: (transfer full): The minimum value. The value isn't + * copied. The returned #GBytes keeps the value alive. * * Since: 8.0.0 */ @@ -612,21 +576,20 @@ GBytes * gparquet_fixed_length_byte_array_statistics_get_min( GParquetFixedLengthByteArrayStatistics *statistics) { - auto priv = GPARQUET_FIXED_LENGTH_BYTE_ARRAY_STATISTICS_GET_PRIVATE(statistics); - if (!priv->min) { - auto parquet_statistics = - gparquet_typed_statistics_get_raw(statistics); - priv->min = g_bytes_new_static(parquet_statistics->min().ptr, - parquet_statistics->descr()->type_length()); - } - return priv->min; + auto parquet_statistics = + gparquet_typed_statistics_get_raw(statistics); + return g_bytes_new_with_free_func(parquet_statistics->min().ptr, + parquet_statistics->descr()->type_length(), + g_object_unref, + g_object_ref(statistics)); } /** * gparquet_fixed_length_byte_array_statistics_get_max: * @statistics: A #GParquetFixedLengthByteArrayStatistics. * - * Returns: (transfer none): The maximum value. + * Returns: (transfer full): The maximum value. The value isn't + * copied. The returned #GBytes keeps the value alive. * * Since: 8.0.0 */ @@ -634,20 +597,19 @@ GBytes * gparquet_fixed_length_byte_array_statistics_get_max( GParquetFixedLengthByteArrayStatistics *statistics) { - auto priv = GPARQUET_FIXED_LENGTH_BYTE_ARRAY_STATISTICS_GET_PRIVATE(statistics); - if (!priv->max) { - auto parquet_statistics = - gparquet_typed_statistics_get_raw(statistics); - priv->max = g_bytes_new_static(parquet_statistics->max().ptr, - parquet_statistics->descr()->type_length()); - } - return priv->max; + auto parquet_statistics = + gparquet_typed_statistics_get_raw(statistics); + return g_bytes_new_with_free_func(parquet_statistics->max().ptr, + parquet_statistics->descr()->type_length(), + g_object_unref, + g_object_ref(statistics)); } G_END_DECLS GParquetStatistics * -gparquet_statistics_new_raw(std::shared_ptr *parquet_statistics) +gparquet_statistics_new_raw(std::shared_ptr *parquet_statistics, + GParquetColumnChunkMetadata *metadata) { GType type = GPARQUET_TYPE_STATISTICS; switch ((*parquet_statistics)->physical_type()) { @@ -675,8 +637,8 @@ gparquet_statistics_new_raw(std::shared_ptr *parquet_statis default: break; } - auto statistics = - GPARQUET_STATISTICS(g_object_new(type, "statistics", parquet_statistics, NULL)); + auto statistics = GPARQUET_STATISTICS( + g_object_new(type, "statistics", parquet_statistics, "metadata", metadata, NULL)); return statistics; } diff --git a/c_glib/parquet-glib/statistics.hpp b/c_glib/parquet-glib/statistics.hpp index 48574217d8aa..b6a9f8564770 100644 --- a/c_glib/parquet-glib/statistics.hpp +++ b/c_glib/parquet-glib/statistics.hpp @@ -21,9 +21,11 @@ #include +#include #include GParquetStatistics * -gparquet_statistics_new_raw(std::shared_ptr *parquet_statistics); +gparquet_statistics_new_raw(std::shared_ptr *parquet_statistics, + GParquetColumnChunkMetadata *metadata); std::shared_ptr gparquet_statistics_get_raw(GParquetStatistics *statistics); diff --git a/c_glib/test/test-binary-array.rb b/c_glib/test/test-binary-array.rb index 0dcaf4eef60c..7b759f6c31b9 100644 --- a/c_glib/test/test-binary-array.rb +++ b/c_glib/test/test-binary-array.rb @@ -33,8 +33,9 @@ def test_value data = "\x00\x01\x02" builder = Arrow::BinaryArrayBuilder.new builder.append_value(data) - array = builder.finish - assert_equal(data, array.get_value(0).to_s) + value = builder.finish.get_value(0) + GC.start + assert_equal(data, value.to_s) end def test_buffer @@ -43,8 +44,9 @@ def test_buffer builder = Arrow::BinaryArrayBuilder.new builder.append_value(data1) builder.append_value(data2) - array = builder.finish - assert_equal(data1 + data2, array.buffer.data.to_s) + data = builder.finish.buffer.data + GC.start + assert_equal(data1 + data2, data.to_s) end def test_offsets_buffer @@ -53,9 +55,10 @@ def test_offsets_buffer builder = Arrow::BinaryArrayBuilder.new builder.append_value(data1) builder.append_value(data2) - array = builder.finish + data = builder.finish.offsets_buffer.data + GC.start byte_per_offset = 4 assert_equal([0, 2, 5].pack("l*"), - array.offsets_buffer.data.to_s[0, byte_per_offset * 3]) + data.to_s[0, byte_per_offset * 3]) end end diff --git a/c_glib/test/test-binary-view-array.rb b/c_glib/test/test-binary-view-array.rb index e1c97ecdced3..a85b9083b108 100644 --- a/c_glib/test/test-binary-view-array.rb +++ b/c_glib/test/test-binary-view-array.rb @@ -16,6 +16,8 @@ # under the License. class TestBinaryViewArray < Test::Unit::TestCase + include Helper::Buildable + def test_new short_binary_data = "test" short_view_buffer_space = 12 @@ -37,4 +39,15 @@ def test_new end assert_equal(short_binary_data, binary_view_array.get_value(0).to_s) end + + def test_value + # TODO: Implement Arrow::BinaryViewArrayBuilder and use it + data = "\x00\x01\x02" * 10 + value = + build_binary_array([data]). + cast(Arrow::BinaryViewDataType.new). + get_value(0) + GC.start + assert_equal(data, value.to_s) + end end diff --git a/c_glib/test/test-fixed-size-binary-array.rb b/c_glib/test/test-fixed-size-binary-array.rb index 8de32c57a622..24217f9b0328 100644 --- a/c_glib/test/test-fixed-size-binary-array.rb +++ b/c_glib/test/test-fixed-size-binary-array.rb @@ -47,13 +47,19 @@ def test_byte_width end def test_value - array = build_fixed_size_binary_array(@data_type, ["0123"]) - assert_equal("0123", array.get_value(0).to_s) + data_type = Arrow::FixedSizeBinaryDataType.new(40) + values = ["0123" * 10] + value = build_fixed_size_binary_array(data_type, values).get_value(0) + GC.start + assert_equal(values[0], value.to_s) end def test_values_bytes - array = build_fixed_size_binary_array(@data_type, - ["0123", "abcd", "0000"]) - assert_equal("0123abcd0000", array.values_bytes.to_s) + data_type = Arrow::FixedSizeBinaryDataType.new(40) + values = ["0123" * 10, "abcd" * 10, "0000" * 10] + values_bytes = + build_fixed_size_binary_array(data_type, values).values_bytes + GC.start + assert_equal(values.join(""), values_bytes.to_s) end end diff --git a/c_glib/test/test-resizable-buffer.rb b/c_glib/test/test-resizable-buffer.rb index 84d95dec573f..deb23d779fa0 100644 --- a/c_glib/test/test-resizable-buffer.rb +++ b/c_glib/test/test-resizable-buffer.rb @@ -29,4 +29,24 @@ def test_reserve @buffer.reserve(1) assert_equal(64, @buffer.capacity) end + + def test_data + data = "Hello" * 10 + @buffer.resize(data.bytesize) + @buffer.set_data(0, data) + buffer_data = @buffer.data + @buffer = nil + GC.start + assert_equal(data, buffer_data.to_s) + end + + def test_mutable_data + data = "Hello" * 10 + @buffer.resize(data.bytesize) + @buffer.set_data(0, data) + buffer_data = @buffer.mutable_data + @buffer = nil + GC.start + assert_equal(data, buffer_data.to_s) + end end diff --git a/c_glib/test/test-string-view-array.rb b/c_glib/test/test-string-view-array.rb index e11474d3caa4..27c992a32088 100644 --- a/c_glib/test/test-string-view-array.rb +++ b/c_glib/test/test-string-view-array.rb @@ -16,6 +16,8 @@ # under the License. class TestStringViewArray < Test::Unit::TestCase + include Helper::Buildable + def test_new short_string_data = "test" short_view_buffer_space = 12 @@ -37,4 +39,15 @@ def test_new end assert_equal(short_string_data, string_view_array.get_value(0).to_s) end + + def test_value + # TODO: Implement Arrow::StringViewArrayBuilder and use it + data = "Hello" * 10 + value = + build_string_array([data]). + cast(Arrow::StringViewDataType.new). + get_value(0) + GC.start + assert_equal(data, value.to_s) + end end diff --git a/ci/scripts/c_glib_test.sh b/ci/scripts/c_glib_test.sh index dd30ebb2f403..bf385fe8dbd9 100755 --- a/ci/scripts/c_glib_test.sh +++ b/ci/scripts/c_glib_test.sh @@ -40,7 +40,16 @@ run_test_args=() if [ -n "${RUNNER_DEBUG}" ]; then run_test_args+=(-v) fi -ruby test/run-test.rb "${run_test_args[@]}" +if [[ "$(uname -s)" == "Linux" ]]; then + env \ + ARROW_DEBUG_MEMORY_POOL=none \ + ARROW_DEFAULT_MEMORY_POOL=system \ + ASAN_OPTIONS="detect_leaks=0:use_sigaltstack=0" \ + LD_PRELOAD="$(gcc -print-file-name=libasan.so) libstdc++.so.6" \ + ruby test/run-test.rb "${run_test_args[@]}" +else + ruby test/run-test.rb "${run_test_args[@]}" +fi if [[ "$(uname -s)" == "Linux" ]]; then # TODO(kszucs): on osx it fails to load 'lgi.corelgilua51' despite that lgi