ARROW-7625: [Parquet][GLib] Add support for writer properties #6336

shiro615 · 2020-02-01T16:24:10Z

No description provided.

github-actions · 2020-02-01T16:31:29Z

https://issues.apache.org/jira/browse/ARROW-7625

kou

parquet::WriterProperties::Builder API is useful for C++ because we can use method chain to build properties:

builder
  .memory_pool(pool)
  .enable_dictionary()
  .data_pagesize(1024);

But this API isn't useful in C because we can't use method chain in C:

gparquet_writer_properties_data_pagesize(
  gparquet_writer_properties_enable_dictionary(
    gparquet_writer_properties_memory_pool(builder, pool)
  ),
  1024);

Or

gparquet_writer_properties_builder_memory_pool(builder, pool);
gparquet_writer_properties_builder_enable_dictionary(builder);
gparquet_writer_properties_builder_data_pagesize(builder, 1024);

So we don't need to provide the builder API to users.

We can just use parquet::WriterProperties::Builder internally. We can provide just accessor API:

gparquet_writer_properties_set_memory_pool(properties, pool);
gparquet_writer_properties_enable_dictionary(properties);
gparquet_writer_properties_set_data_pagesize(properties, 1024);

And we can build parquet::WriterProperties when it's needed:

gparquet_arrow_file_writer_new_*()
{
  ...
  auto parquet_writer_properties =
    gparquet_writer_properties_get_raw(writer_properties);
  ...
}

Could you change the API or do I push base implementation?

kou · 2020-02-02T21:49:00Z

Could you enable Parquet on macOS CI?

diff --git a/.github/workflows/ruby.yml b/.github/workflows/ruby.yml
index bc28c6f54..f9ba56b67 100644
--- a/.github/workflows/ruby.yml
+++ b/.github/workflows/ruby.yml
@@ -85,6 +85,7 @@ jobs:
       ARROW_HOME: /usr/local
       ARROW_JEMALLOC: OFF
       ARROW_ORC: OFF
+      ARROW_PARQUET: ON
       ARROW_WITH_BROTLI: ON
       ARROW_WITH_LZ4: ON
       ARROW_WITH_SNAPPY: ON

shiro615 · 2020-02-04T15:33:48Z

Thank you for your review.
I'll try to address them.

shiro615 · 2020-02-05T16:47:51Z

@kou
fbe3f66
Could you please confirm my understanding?
I'll add the other properties in this PR if this change is fine.

kou

Could you check my comments?

kou · 2020-02-06T03:41:05Z

c_glib/parquet-glib/arrow-file-writer.cpp

+
+static void
+gparquet_writer_properties_init(GParquetWriterProperties *object)
+{


We can always create parquet::WriterPropertiesBuilder here.
We don't need to receive it as an argument of g_object_new().

kou · 2020-02-06T03:43:49Z

c_glib/parquet-glib/arrow-file-writer.cpp

+gparquet_writer_properties_get_raw(GParquetWriterProperties *properties)
+{
+  auto priv = GPARQUET_WRITER_PROPERTIES_GET_PRIVATE(properties);
+  return priv->builder->build();


I want to cache build() result.

if (priv->changed) { priv->properties = priv->builder->build(); } return priv->properties;

kou · 2020-02-06T03:54:31Z

c_glib/parquet-glib/arrow-file-writer.cpp

+gparquet_writer_properties_get_compression(GParquetWriterProperties *properties)
+{
+  auto priv = GPARQUET_WRITER_PROPERTIES_GET_PRIVATE(properties);
+  return priv->compression_type;


I don't want to keep duplicated information in this object.

auto parquet_properties = gparquet_writer_properties_get_raw(properties); auto parquet_column_path = parquet::Schema::ColumnPath::FromDotString(dotstring); // Or receive as an argument auto arrow_compression = parquet_properties->compression(parquet_column_path); return garrow_compression_type_from_raw(arrow_compression);

shiro615 · 2020-02-06T15:42:33Z

Thank you for your comments.
I'll fix them in this week.

ziggythehamster · 2020-02-06T20:59:05Z

Nice catch with the other configurables (dictionary, memory pool) and exposing the per-column settings. I was happy with:

parquet::WriterProperties::Builder builder;
builder.compression(arrow::Compression::SNAPPY);
auto parquet_writer_properties = builder.build();

:P

kou · 2020-02-07T00:44:56Z

@ziggythehamster What do you mean? Do you want to use parquet::WriterProperties built by C++ with Apache Arrow GLib?

shiro615 · 2020-02-08T00:44:33Z

I hope I've addressed all review comments. In addition to them, I've added some properties.

kou

Could you check my comments?

kou · 2020-02-09T21:18:38Z

c_glib/parquet-glib/arrow-file-writer.cpp

+}
+
+/**
+ * gparquet_writer_properties_get_compression:


Could you append _dot_string?
Because we will add parquet::Schema::ColumnPath version later.

kou · 2020-02-09T21:18:46Z

c_glib/parquet-glib/arrow-file-writer.cpp

+/**
+ * gparquet_writer_properties_get_compression:
+ * @properties: A #GParquetWriterProperties.
+ * @dotstring: The dot string path.


@dot_string

kou · 2020-02-09T21:20:05Z

c_glib/parquet-glib/arrow-file-writer.cpp

+/**
+ * gparquet_writer_properties_dictionary_enabled:
+ * @properties: A #GParquetWriterProperties.
+ *


@dot_string is missing.

kou · 2020-02-09T21:22:45Z

c_glib/parquet-glib/arrow-file-writer.cpp

+}
+
+/**
+ * gparquet_writer_properties_dictionary_enabled:


Could you add is_ for predicate?
gparquet_writer_properties_is_dictionary_enabled.

kou · 2020-02-09T21:23:32Z

c_glib/parquet-glib/arrow-file-writer.cpp

+/**
+ * gparquet_writer_properties_set_dictionary_pagesize_limit:
+ * @properties: A #GParquetWriterProperties.
+ * @dictionary_pagesize_limit: The dictionary page size limit.


We can simplify this to @limit.

kou · 2020-02-09T21:26:24Z

c_glib/parquet-glib/arrow-file-writer.cpp

+/**
+ * gparquet_writer_properties_set_data_pagesize:
+ * @properties: A #GParquetWriterProperties.
+ * @data_pagesize: The data page size.


Should we use page_size instead of pagesize?

kou · 2020-02-09T21:27:05Z

c_glib/parquet-glib/arrow-file-writer.cpp

@@ -133,18 +398,29 @@ gparquet_arrow_file_writer_class_init(GParquetArrowFileWriterClass *klass)
 GParquetArrowFileWriter *
 gparquet_arrow_file_writer_new_arrow(GArrowSchema *schema,
                                     GArrowOutputStream *sink,
+                                     GParquetWriterProperties *writer_properties,


This breaks API backward compatibility but it may be acceptable...

kou · 2020-02-09T21:27:48Z

c_glib/parquet-glib/arrow-file-writer.cpp

+  if (priv->changed) {
+    priv->properties = priv->builder->build();
+  }
+  priv->changed = FALSE;


Could you put this to if (priv->changed) { clause?

kou · 2020-02-09T21:29:17Z

c_glib/test/parquet/test-writer-properties.rb

+  def test_compression
+    @properties.compression = :gzip
+    assert_equal(Arrow::CompressionType.new("gzip"),
+                 @properties.get_compression("a_column"))


Could you use not-specified or something instead of a_column?

kou · 2020-02-09T21:31:40Z

c_glib/parquet-glib/arrow-file-writer.cpp

+ */
+void
+gparquet_writer_properties_set_compression(GParquetWriterProperties *properties,
+                                           GArrowCompressionType compression_type)


How about adding nullable path or dot_string?

gparquet_writer_properties_set_compression((GParquetWriterProperties *properties, const gchar *path, GArrowCompressionType compression_type)

I've added nullable path.

shiro615 · 2020-02-15T18:13:27Z

Thank you for your review. I think I've addressed them.

…_init()

…t_compression()

…properties_get_compression_dot_string

…er_properties_is_dictionary_enabled

We will use "column_path" for Parquet::Schema::ColumnPath.

kou

+1

@shiro615 Sorry for my late review.
I pushed some changes:

Add the "path" argument to enable_dictionary() and disable_dictionary().
Rename the "dot_string" argument to "path". Sorry. How about using "path" as "dot string" in Parquet GLib? We will use "column_path" when we need to export Parquet::Schema::ColumnPath.

Could you confirm them?

shiro615 · 2020-02-27T04:33:57Z

Thank you for your review and suggestion.

How about using "path" as "dot string" in Parquet GLib?

It sounds good for me too.

I'll merge this.

kou requested changes Feb 2, 2020

View reviewed changes

kou reviewed Feb 6, 2020

View reviewed changes

kszucs force-pushed the master branch from b18ed44 to e79c251 Compare February 7, 2020 07:41

kszucs force-pushed the glib-parquet-writer-options branch from fbe3f66 to 0fce7f1 Compare February 7, 2020 10:09

shiro615 force-pushed the glib-parquet-writer-options branch from 31f431a to 2ed0e47 Compare February 8, 2020 00:19

shiro615 requested a review from kou February 8, 2020 00:45

kou requested changes Feb 9, 2020

View reviewed changes

shiro615 force-pushed the glib-parquet-writer-options branch 4 times, most recently from 262290b to 24f723e Compare February 15, 2020 17:27

shiro615 requested a review from kou February 15, 2020 18:13

shiro615 added 9 commits February 27, 2020 06:15

ARROW-7625: [Parquet][GLib] Add support for writer properties

739155d

Omit test cases if Parquet is not installed

27f043d

Use parquet::WriterProperties::Builder internally

5b0c40e

Create parquet::WriterPropertiesBuilder in gparquet_writer_properties…

1a409cf

…_init()

Call WriterProperties::compression() in gparquet_writer_properties_ge…

901c030

…t_compression()

Cache build() result

be43048

Fix test cases

79113cf

Add other properties

32a3222

Enable Parquet on macOS CI

a3a88a0

shiro615 and others added 13 commits February 27, 2020 06:15

Rename gparquet_writer_properties_get_compression to gparquet_writer_…

8770cd3

…properties_get_compression_dot_string

Fix argument names

ce4b5df

Rename gparquet_writer_properties_dictionary_enabled to gparquet_writ…

e52a020

…er_properties_is_dictionary_enabled

Simplify argument names

0bdc266

Use page_size instead of pagesize

3c63e62

Refactor

6222af9

Use not-specified instead of a_column

579a0e0

Add path argument

d425b79

Add const for path

2350297

Fix test cases

5c50464

Add version macros

5886091

Delete an extra line

739c6ce

Add a newline as paragraph separator.

c0ba797

kou force-pushed the glib-parquet-writer-options branch from 24f723e to c0ba797 Compare February 26, 2020 21:36

kou added 2 commits February 27, 2020 06:49

Add path argument to enable_dictionary() and disable_dictionary()

03ec917

Use "path" for "dot string" in Parquet GLib

86b8780

We will use "column_path" for Parquet::Schema::ColumnPath.

kou approved these changes Feb 26, 2020

View reviewed changes

shiro615 closed this in ccbdf3c Feb 27, 2020

shiro615 deleted the glib-parquet-writer-options branch February 27, 2020 04:35

asfimport mentioned this pull request Feb 27, 2020

[GLib] Parquet GLib and Red Parquet (Ruby) do not allow specifying compression type #23877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-7625: [Parquet][GLib] Add support for writer properties #6336

ARROW-7625: [Parquet][GLib] Add support for writer properties #6336

shiro615 commented Feb 1, 2020

github-actions bot commented Feb 1, 2020

kou left a comment

kou commented Feb 2, 2020

shiro615 commented Feb 4, 2020

shiro615 commented Feb 5, 2020

kou left a comment

kou Feb 6, 2020

kou Feb 6, 2020

kou Feb 6, 2020

shiro615 commented Feb 6, 2020

ziggythehamster commented Feb 6, 2020

kou commented Feb 7, 2020

shiro615 commented Feb 8, 2020 •

edited

Loading

kou left a comment

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

kou Feb 9, 2020

shiro615 Feb 11, 2020

shiro615 commented Feb 15, 2020

kou left a comment

shiro615 commented Feb 27, 2020

ARROW-7625: [Parquet][GLib] Add support for writer properties #6336

ARROW-7625: [Parquet][GLib] Add support for writer properties #6336

Conversation

shiro615 commented Feb 1, 2020

github-actions bot commented Feb 1, 2020

kou left a comment

Choose a reason for hiding this comment

kou commented Feb 2, 2020

shiro615 commented Feb 4, 2020

shiro615 commented Feb 5, 2020

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiro615 commented Feb 6, 2020

ziggythehamster commented Feb 6, 2020

kou commented Feb 7, 2020

shiro615 commented Feb 8, 2020 • edited Loading

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiro615 commented Feb 15, 2020

kou left a comment

Choose a reason for hiding this comment

shiro615 commented Feb 27, 2020

shiro615 commented Feb 8, 2020 •

edited

Loading