GH-41608: [C++][Python] Extends the add_key_value to parquet::arrow and PyArrow #41633

mapleFU · 2024-05-13T06:04:57Z

Rationale for this change

The previous pr ( #34889 ) add a AddKeyValueMetadata to FileWriter. And now we should export it to Parquet Arrow and Python API.

What changes are included in this PR?

Add AddKeyValueMetadata in parquet::arrow
Add add_key_value_metadata in pyarrow
testing

Are these changes tested?

Yes

Are there any user-facing changes?

New api allowing add key-value metadata to Parquet file

GitHub Issue: Enable updating Parquet key-value metadata of opened ParquetWriter before closing #41608

github-actions · 2024-05-13T06:05:23Z

⚠️ GitHub issue #41608 has been automatically assigned in GitHub to PR creator.

mapleFU · 2024-05-24T15:29:27Z

cpp/src/parquet/type_fwd.h

 class ArrowReaderProperties;

 class WriterProperties;
-class WriterPropertiesBuilder;


We don't actually have this class, lol

Sigh, maybe this is for R language ( see r/src/parquet.cpp), revert it back

wgtmac · 2024-05-27T01:35:53Z

cpp/src/parquet/arrow/writer.h

+  ///
+  /// WARNING: If `store_schema` is enabled, `ARROW:schema` would be stored
+  /// in the key-value metadata. Overwriting this key would result in
+  /// `store_schema` unusable during read.


Suggested change

/// `store_schema` unusable during read.

/// undefined behavior of `ARROW:schema` during read.

Seems this cannot be used here? And if schema cannot being read, it would use "physical" schema?

Let's not use the loaded term "undefined behavior" here? I think the original wording is mostly fine, e.g.:

Suggested change

/// `store_schema` unusable during read.

/// `store_schema` being unusable during read.

mapleFU · 2024-05-27T14:31:49Z

I've no idea why R language CI failed

pitrou · 2024-05-28T15:00:33Z

cpp/src/parquet/arrow/arrow_metadata_test.cc

+
+  auto kv_meta = std::make_shared<KeyValueMetadata>();
+  kv_meta->Append("test_key_1", "test_value_1");
+  kv_meta->Append("test_key_2", "test_value_2_");


The difference between test_value_2 and test_value_2_ is very obscure and easy to miss. Can you make this test clearer?

// <test_key_2, test_value_2_temp> would be overwritten later. kv_meta->Append("test_key_2", "test_value_2_temp");

Change to the code here

pitrou · 2024-05-28T15:01:28Z

cpp/src/parquet/arrow/arrow_metadata_test.cc

+  const auto& key_value_metadata = writer->metadata()->key_value_metadata();
+  ASSERT_TRUE(nullptr != key_value_metadata);
+
+  // Verify keys that were added before file writer was closed are present.


Can we instead read the file to make sure the metadata is here?

Sure, but I don't think they would be different. I'll verify them twice ( by read from writer->metadata()->key_value_metadata() and file)

pitrou · 2024-05-28T15:02:39Z

cpp/src/parquet/arrow/writer.h

+  ///
+  /// WARNING: If `store_schema` is enabled, `ARROW:schema` would be stored
+  /// in the key-value metadata. Overwriting this key would result in
+  /// `store_schema` unusable during read.


Let's not use the loaded term "undefined behavior" here? I think the original wording is mostly fine, e.g.:

Suggested change

/// `store_schema` unusable during read.

/// `store_schema` being unusable during read.

cpp/src/parquet/arrow/writer.h

pitrou · 2024-05-28T15:04:49Z

@jorisvandenbossche Could you perhaps review the Cython/Python parts of this PR?

mapleFU · 2024-06-04T05:22:15Z

@AlenkaF @jorisvandenbossche @pitrou Would you mind take a look?

AlenkaF

Thank you for adding this!

Python/Cython part LGTM 👍 with only one suggestion: I think the python test fits better in tests/parquet/test_parquet_writer.py.

mapleFU · 2024-06-04T10:34:03Z

Migrate to test_parquet_writer.py now. @pitrou would you mind revisit C++ part or merge this?

pitrou

LGTM, just two nits

pitrou · 2024-06-04T12:32:03Z

python/pyarrow/_parquet.pxd

                                        COutputStream, CCacheOptions,
                                        TimeUnit, CRecordBatchReader)
-from pyarrow.lib cimport _Weakrefable
+from pyarrow.lib cimport (_Weakrefable, pyarrow_unwrap_metadata, KeyValueMetadata)


Note that the new imports are only required in _parquet.pyx AFAICT.

pitrou · 2024-06-04T12:35:27Z

python/pyarrow/parquet/core.py

+
+        Parameters
+        ----------
+        key_value_metadata : {Key, Value}


Let's reuse the same conventions as in other docstrings:

Suggested change

key_value_metadata : {Key, Value}

key_value_metadata : dict

Keys and values must be string-like / coercible to bytes

mapleFU · 2024-06-04T14:28:52Z

@pitrou comment fixed

pitrou · 2024-06-04T14:38:18Z

@mapleFU Feel free to merge if CI is fine.

mapleFU · 2024-06-04T14:40:16Z

CI failed is unrelated, merge

conbench-apache-arrow · 2024-06-11T13:39:58Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d02a91b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 10 possible false positives for unstable benchmarks that are known to sometimes produce them.

Extends the append_key_value to parquet::arrow

7c699fb

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels May 13, 2024

mapleFU mentioned this pull request May 22, 2024

GH-41766: [C++][Parquet] Copy key value metadata when store_schema is false #41769

Open

add basic tests for arrow metadata

c6917c0

mapleFU commented May 24, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic

5f3ef38

github-actions bot added the Component: Python label May 24, 2024

Fix the parquet test

499f230

mapleFU changed the title ~~GH-41608: [C++][Python][Parquet] Extends the append_key_value to parquet::arrow and PySDK~~ GH-41608: [C++][Python][Parquet] Extends the append_key_value to parquet::arrow and PyArrow May 24, 2024

mapleFU requested review from AlenkaF and pitrou May 24, 2024 16:15

mapleFU marked this pull request as ready for review May 24, 2024 16:15

mapleFU requested a review from wgtmac as a code owner May 24, 2024 16:15

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from 034d5f8 to b319fc8 Compare May 24, 2024 16:20

mapleFU changed the title ~~GH-41608: [C++][Python][Parquet] Extends the append_key_value to parquet::arrow and PyArrow~~ GH-41608: [C++][Python][Parquet] Extends the add_key_value to parquet::arrow and PyArrow May 24, 2024

mapleFU changed the title ~~GH-41608: [C++][Python][Parquet] Extends the add_key_value to parquet::arrow and PyArrow~~ GH-41608: [C++][Python] Extends the add_key_value to parquet::arrow and PyArrow May 24, 2024

update format

48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfb Compare May 24, 2024 17:03

wgtmac reviewed May 27, 2024

View reviewed changes

mapleFU added 2 commits May 28, 2024 00:02

Merge branch 'main' into parquet-export-kvmeta-to-arrow-and-python

aef3d04

remove rubbish code

93b3bbb

pitrou reviewed May 28, 2024

View reviewed changes

apply suggestions

a9155c8

mapleFU added 2 commits June 4, 2024 12:46

Merge branch 'main' into parquet-export-kvmeta-to-arrow-and-python

090f9dd

Revert remove builder

8a10742

AlenkaF approved these changes Jun 4, 2024

View reviewed changes

Move test to test_parquet_writer.py

8fdf63f

pitrou approved these changes Jun 4, 2024

View reviewed changes

mapleFU added 2 commits June 4, 2024 21:17

apply suggestions

e48826e

fix compile

970fefb

mapleFU merged commit d02a91b into apache:main Jun 4, 2024

mapleFU removed the awaiting committer review Awaiting committer review label Jun 4, 2024

mapleFU mentioned this pull request Jun 4, 2024

Enable updating Parquet key-value metadata of opened ParquetWriter before closing #41608

Closed

mapleFU deleted the parquet-export-kvmeta-to-arrow-and-python branch June 4, 2024 14:41

	/// `store_schema` unusable during read.
	/// undefined behavior of `ARROW:schema` during read.

	/// `store_schema` unusable during read.
	/// `store_schema` being unusable during read.

	key_value_metadata : {Key, Value}
	key_value_metadata : dict
	Keys and values must be string-like / coercible to bytes

GH-41608: [C++][Python] Extends the add_key_value to parquet::arrow and PyArrow #41633

GH-41608: [C++][Python] Extends the add_key_value to parquet::arrow and PyArrow #41633

Uh oh!

Conversation

mapleFU commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented May 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented May 28, 2024

Uh oh!

mapleFU commented Jun 4, 2024

Uh oh!

AlenkaF left a comment

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jun 4, 2024

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jun 4, 2024

Uh oh!

pitrou commented Jun 4, 2024

Uh oh!

mapleFU commented Jun 4, 2024

Uh oh!

conbench-apache-arrow bot commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mapleFU commented May 13, 2024 •

edited

Loading