Skip to content

Commit

Permalink
apacheGH-35266: [GLib][Parquet] Fix a GC bug that parent metadata ref…
Browse files Browse the repository at this point in the history 
…erence is missing in sub metadata (apache#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: apache#35266

Authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
  • Loading branch information
@kou @rtpsw
kou authored and rtpsw committed May 16, 2023
1 parent 4482fa4 commit 2063b44
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 17 deletions.
64 changes: 51 additions & 13 deletions c_glib/parquet-glib/metadata.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -213,6 +230,7 @@ gparquet_column_chunk_metadata_get_statistics(

struct GParquetRowGroupMetadataPrivate {
parquet::RowGroupMetaData *metadata;
GParquetFileMetadata *owner;
};

G_DEFINE_TYPE_WITH_PRIVATE(GParquetRowGroupMetadata,
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
7 changes: 5 additions & 2 deletions c_glib/parquet-glib/metadata.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
1 change: 0 additions & 1 deletion c_glib/test/parquet/test-column-chunk-metadata.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ def setup
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
Expand Down
1 change: 0 additions & 1 deletion c_glib/test/parquet/test-row-group-metadata.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ def setup
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
Expand Down

0 comments on commit 2063b44

Please sign in to comment.