GH-41608: [C++][Python] Extends the add_key_value to parquet::arrow and PyArrow by mapleFU · Pull Request #41633 · apache/arrow

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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.

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 #41608has been automatically assigned in GitHub to PR creator.

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

 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

+ ///
+ /// 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

+
+ 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

+ 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

+ ///
+ /// 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.

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

 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

+
+ 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
Keysandvaluesmustbestring-like/coercibletobytes

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-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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

Extends the append_key_value to parquet::arrow
7c699fb

github-actionsBot 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-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 24, 2024

add py logic
5f3ef38

github-actionsBot 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 PyArrowMay 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 b319fc8Compare 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 PyArrowMay 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 PyArrowMay 24, 2024

update format
48d9cfb

mapleFU force-pushed the parquet-export-kvmeta-to-arrow-and-python branch from b319fc8 to 48d9cfbCompare 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:mainJun 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
	Keysandvaluesmustbestring-like/coercibletobytes

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-actionsBot 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-arrowBot commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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-actionsBot commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mapleFU commented May 13, 2024•
edited
Loading

mapleFU commented May 13, 2024•
edited
Loading