This is an automated email from the ASF dual-hosted git repository.
kou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new f4bd43d28e GH-35266: [GLib][Parquet] Fix a GC bug that parent metadata
reference is missing in sub metadata (#35286)
f4bd43d28e is described below
commit f4bd43d28eae13a48d5225ea44a16646ef093155
Author: Sutou Kouhei <[email protected]>
AuthorDate: Sun Apr 23 14:33:53 2023 +0900
GH-35266: [GLib][Parquet] Fix a GC bug that parent metadata reference is
missing in sub metadata (#35286)
### Rationale for this change
`GParquetColumnChunkMetadata` must not be GC-ed while the parent
`GParquetRowGroupMetadata` is alive.
`GParquetRowGroupMetadata` must not be GC-ed while the parent
`GParquetFileMetadata` is alive.
### What changes are included in this PR?
Add missing parent metadata references to sub metadata.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
* Closes: #35266
Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
---
c_glib/parquet-glib/metadata.cpp | 64 ++++++++++++++++++-----
c_glib/parquet-glib/metadata.hpp | 7 ++-
c_glib/test/parquet/test-column-chunk-metadata.rb | 1 -
c_glib/test/parquet/test-row-group-metadata.rb | 1 -
4 files changed, 56 insertions(+), 17 deletions(-)
diff --git a/c_glib/parquet-glib/metadata.cpp b/c_glib/parquet-glib/metadata.cpp
index cb78ed0a82..54583340ad 100644
--- a/c_glib/parquet-glib/metadata.cpp
+++ b/c_glib/parquet-glib/metadata.cpp
@@ -38,10 +38,12 @@ G_BEGIN_DECLS
struct GParquetColumnChunkMetadataPrivate {
parquet::ColumnChunkMetaData *metadata;
+ GParquetRowGroupMetadata *owner;
};
enum {
PROP_METADATA = 1,
+ PROP_OWNER,
};
G_DEFINE_TYPE_WITH_PRIVATE(GParquetColumnChunkMetadata,
@@ -54,11 +56,14 @@ G_DEFINE_TYPE_WITH_PRIVATE(GParquetColumnChunkMetadata,
GPARQUET_COLUMN_CHUNK_METADATA(object)))
static void
-gparquet_column_chunk_metadata_finalize(GObject *object)
+gparquet_column_chunk_metadata_dispose(GObject *object)
{
auto priv = GPARQUET_COLUMN_CHUNK_METADATA_GET_PRIVATE(object);
- delete priv->metadata;
-
G_OBJECT_CLASS(gparquet_column_chunk_metadata_parent_class)->finalize(object);
+ if (priv->owner) {
+ g_object_unref(priv->owner);
+ priv->owner = nullptr;
+ }
+ G_OBJECT_CLASS(gparquet_column_chunk_metadata_parent_class)->dispose(object);
}
static void
@@ -74,6 +79,9 @@ gparquet_column_chunk_metadata_set_property(GObject *object,
priv->metadata =
static_cast<parquet::ColumnChunkMetaData *>(g_value_get_pointer(value));
break;
+ case PROP_OWNER:
+ priv->owner = GPARQUET_ROW_GROUP_METADATA(g_value_dup_object(value));
+ break;
default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
break;
@@ -90,15 +98,24 @@ gparquet_column_chunk_metadata_class_init(
GParquetColumnChunkMetadataClass *klass)
{
auto gobject_class = G_OBJECT_CLASS(klass);
- gobject_class->finalize = gparquet_column_chunk_metadata_finalize;
+ gobject_class->dispose = gparquet_column_chunk_metadata_dispose;
gobject_class->set_property = gparquet_column_chunk_metadata_set_property;
GParamSpec *spec;
- spec = g_param_spec_pointer("metadata", "Metadata",
+ spec = g_param_spec_pointer("metadata",
+ "Metadata",
"The raw parquet::ColumnChunkMetaData *",
static_cast<GParamFlags>(G_PARAM_WRITABLE |
G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_METADATA, spec);
+
+ spec = g_param_spec_object("owner",
+ "Owner",
+ "The row group metadata that owns this metadata",
+ GPARQUET_TYPE_ROW_GROUP_METADATA,
+ static_cast<GParamFlags>(G_PARAM_WRITABLE |
+ G_PARAM_CONSTRUCT_ONLY));
+ g_object_class_install_property(gobject_class, PROP_OWNER, spec);
}
/**
@@ -213,6 +230,7 @@ gparquet_column_chunk_metadata_get_statistics(
struct GParquetRowGroupMetadataPrivate {
parquet::RowGroupMetaData *metadata;
+ GParquetFileMetadata *owner;
};
G_DEFINE_TYPE_WITH_PRIVATE(GParquetRowGroupMetadata,
@@ -225,11 +243,14 @@ G_DEFINE_TYPE_WITH_PRIVATE(GParquetRowGroupMetadata,
GPARQUET_ROW_GROUP_METADATA(object)))
static void
-gparquet_row_group_metadata_finalize(GObject *object)
+gparquet_row_group_metadata_dispose(GObject *object)
{
auto priv = GPARQUET_ROW_GROUP_METADATA_GET_PRIVATE(object);
- delete priv->metadata;
- G_OBJECT_CLASS(gparquet_row_group_metadata_parent_class)->finalize(object);
+ if (priv->owner) {
+ g_object_unref(priv->owner);
+ priv->owner = nullptr;
+ }
+ G_OBJECT_CLASS(gparquet_row_group_metadata_parent_class)->dispose(object);
}
static void
@@ -245,6 +266,9 @@ gparquet_row_group_metadata_set_property(GObject *object,
priv->metadata =
static_cast<parquet::RowGroupMetaData *>(g_value_get_pointer(value));
break;
+ case PROP_OWNER:
+ priv->owner = GPARQUET_FILE_METADATA(g_value_dup_object(value));
+ break;
default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
break;
@@ -260,7 +284,7 @@ static void
gparquet_row_group_metadata_class_init(GParquetRowGroupMetadataClass *klass)
{
auto gobject_class = G_OBJECT_CLASS(klass);
- gobject_class->finalize = gparquet_row_group_metadata_finalize;
+ gobject_class->finalize = gparquet_row_group_metadata_dispose;
gobject_class->set_property = gparquet_row_group_metadata_set_property;
GParamSpec *spec;
@@ -269,6 +293,14 @@
gparquet_row_group_metadata_class_init(GParquetRowGroupMetadataClass *klass)
static_cast<GParamFlags>(G_PARAM_WRITABLE |
G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_METADATA, spec);
+
+ spec = g_param_spec_object("owner",
+ "Owner",
+ "The file group metadata that owns this metadata",
+ GPARQUET_TYPE_FILE_METADATA,
+ static_cast<GParamFlags>(G_PARAM_WRITABLE |
+ G_PARAM_CONSTRUCT_ONLY));
+ g_object_class_install_property(gobject_class, PROP_OWNER, spec);
}
/**
@@ -335,7 +367,8 @@
gparquet_row_group_metadata_get_column_chunk(GParquetRowGroupMetadata *metadata,
status,
"[parquet][row-group-metadata][get-column-chunk]")) {
return gparquet_column_chunk_metadata_new_raw(
- parquet_column_chunk_metadata.release());
+ parquet_column_chunk_metadata.release(),
+ metadata);
} else {
return NULL;
}
@@ -602,7 +635,8 @@ gparquet_file_metadata_get_row_group(GParquetFileMetadata
*metadata,
END_PARQUET_CATCH_EXCEPTIONS
})();
if (garrow::check(error, status, "[parquet][file-metadata][get-row-group]"))
{
- return
gparquet_row_group_metadata_new_raw(parquet_row_group_metadata.release());
+ return
gparquet_row_group_metadata_new_raw(parquet_row_group_metadata.release(),
+ metadata);
} else {
return NULL;
}
@@ -664,12 +698,14 @@ G_END_DECLS
GParquetColumnChunkMetadata *
gparquet_column_chunk_metadata_new_raw(
- parquet::ColumnChunkMetaData *parquet_metadata)
+ parquet::ColumnChunkMetaData *parquet_metadata,
+ GParquetRowGroupMetadata *owner)
{
auto metadata =
GPARQUET_COLUMN_CHUNK_METADATA(
g_object_new(GPARQUET_TYPE_COLUMN_CHUNK_METADATA,
"metadata", parquet_metadata,
+ "owner", owner,
NULL));
return metadata;
}
@@ -683,11 +719,13 @@
gparquet_column_chunk_metadata_get_raw(GParquetColumnChunkMetadata *metadata)
GParquetRowGroupMetadata *
-gparquet_row_group_metadata_new_raw(parquet::RowGroupMetaData
*parquet_metadata)
+gparquet_row_group_metadata_new_raw(parquet::RowGroupMetaData
*parquet_metadata,
+ GParquetFileMetadata *owner)
{
auto metadata =
GPARQUET_ROW_GROUP_METADATA(g_object_new(GPARQUET_TYPE_ROW_GROUP_METADATA,
"metadata", parquet_metadata,
+ "owner", owner,
NULL));
return metadata;
}
diff --git a/c_glib/parquet-glib/metadata.hpp b/c_glib/parquet-glib/metadata.hpp
index a1704d54bb..279e60ffe8 100644
--- a/c_glib/parquet-glib/metadata.hpp
+++ b/c_glib/parquet-glib/metadata.hpp
@@ -25,12 +25,15 @@
GParquetColumnChunkMetadata *
gparquet_column_chunk_metadata_new_raw(
- parquet::ColumnChunkMetaData *parquet_metadata);
+ parquet::ColumnChunkMetaData *parquet_metadata,
+ GParquetRowGroupMetadata *owner);
parquet::ColumnChunkMetaData *
gparquet_column_chunk_metadata_get_raw(GParquetColumnChunkMetadata *metadata);
GParquetRowGroupMetadata *
-gparquet_row_group_metadata_new_raw(parquet::RowGroupMetaData
*parquet_metadata);
+gparquet_row_group_metadata_new_raw(
+ parquet::RowGroupMetaData *parquet_metadata,
+ GParquetFileMetadata *owner);
parquet::RowGroupMetaData *
gparquet_row_group_metadata_get_raw(GParquetRowGroupMetadata *metadata);
diff --git a/c_glib/test/parquet/test-column-chunk-metadata.rb
b/c_glib/test/parquet/test-column-chunk-metadata.rb
index 81d3dab971..a93fe85bbf 100644
--- a/c_glib/test/parquet/test-column-chunk-metadata.rb
+++ b/c_glib/test/parquet/test-column-chunk-metadata.rb
@@ -45,7 +45,6 @@ class TestParquetColumnChunkMetadata < Test::Unit::TestCase
end
test("#==") do
- omit("parquet::ColumnChunkMetaData::Equals() isn't stable.")
reader = Parquet::ArrowFileReader.new(@file.path)
other_metadata = reader.metadata.get_row_group(0).get_column_chunk(0)
assert do
diff --git a/c_glib/test/parquet/test-row-group-metadata.rb
b/c_glib/test/parquet/test-row-group-metadata.rb
index 575fa19c35..e68cb9d11e 100644
--- a/c_glib/test/parquet/test-row-group-metadata.rb
+++ b/c_glib/test/parquet/test-row-group-metadata.rb
@@ -45,7 +45,6 @@ class TestParquetRowGroupMetadata < Test::Unit::TestCase
end
test("#==") do
- omit("parquet::RowGroupMetaData::Equals() isn't stable.")
reader = Parquet::ArrowFileReader.new(@file.path)
other_metadata = reader.metadata.get_row_group(0)
assert do
