GH-34888: [C++][Parquet] Writer supports adding extra kv meta #34889

wgtmac · 2023-04-04T15:47:40Z

Rationale for this change

Parquet specs support storing key-value metadata provided by the user. However, the parquet-cpp writer can only set it via ParquetFileWriter::Open(). Sometimes user may want to add extra information to it while writing. So it is good to support adding extra key-value metadata any time before closing the file writer.

What changes are included in this PR?

Add a new interface void AddKeyValueMetadata(std::shared_ptr<const KeyValueMetadata> key_value_metadata) to the ParquetFileWriter class. User can now add more key-value metadata to the file if not closed.

Are these changes tested?

Added a new Metadata.TestAddKeyValueMetadata test to verify key-value metadata added before closing the writer are well preserved.

Are there any user-facing changes?

Yes, user can add custom key-value metadata whenever writer is not closed.

Closes: [C++][Parquet] Writer cannot add extra key-value metadata once opened #34888

github-actions · 2023-04-04T15:48:04Z

Closes: [C++][Parquet] Writer cannot add extra key-value metadata once opened #34888

github-actions · 2023-04-04T15:48:07Z

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

wgtmac · 2023-04-07T09:55:40Z

@wjones127 @westonpace Please take a look when you have time. Thanks!

wjones127 · 2023-04-07T23:13:39Z

cpp/src/parquet/metadata.cc

I think we can remove this?

cpp/src/parquet/metadata.h

mapleFU

Rest LGTM

cpp/src/parquet/file_writer.cc

cpp/src/parquet/file_writer.h

cpp/src/parquet/file_writer.cc

cpp/src/parquet/metadata.h

wgtmac · 2023-04-10T16:24:28Z

Fixed it to be backward compatible. Please take a look again. Thanks! @wjones127

wgtmac · 2023-04-11T01:58:24Z

https://github.com/apache/arrow/actions/runs/4662936307/jobs/8253813543?pr=34889 is failing:

+ doxygen
/arrow/cpp/apidoc /
warning: Tag 'OUTPUT_TEXT_DIRECTION' at line 119 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'COLS_IN_ALPHA_INDEX' at line 1118 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'FORMULA_TRANSPARENT' at line 1557 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'LATEX_SOURCE_CODE' at line 1865 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'RTF_SOURCE_CODE' at line 1955 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOCBOOK_PROGRAMLISTING' at line 2060 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'CLASS_DIAGRAMS' at line 2257 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOT_FONTNAME' at line 2299 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOT_FONTSIZE' at line 2306 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
warning: Tag 'DOT_TRANSPARENT' at line 2530 of file 'Doxyfile' has become obsolete.
         To avoid this warning please remove this line from your configuration file or upgrade it using "doxygen -u"
/arrow/cpp/src/parquet/metadata.h:531: error: Found ';' while parsing initializer list! (doxygen could be confused by a macro call without semicolon) (warning treated as error, aborting now)
1
Error: `docker-compose --file /home/runner/work/arrow/arrow/docker-compose.yml run --rm conda-python-docs` exited with a non-zero exit code 1, see the process log above.

wjones127

Just one minor comment, otherwise looks good!

cpp/src/parquet/file_writer.cc

Co-authored-by: Will Jones <[email protected]>

wgtmac · 2023-04-12T02:15:18Z

cpp/src/parquet/metadata.h

 class PARQUET_EXPORT FileMetaDataBuilder {
 public:
-  // API convenience to get a MetaData reader
+  ARROW_DEPRECATED("Deprecated in 12.0.0. Use overload without KeyValueMetadata instead.")


Suggested change

ARROW_DEPRECATED("Deprecated in 12.0.0. Use overload without KeyValueMetadata instead.")

ARROW_DEPRECATED("Deprecated in 13.0.0. Use overload without KeyValueMetadata instead.")

Should I change this since it may not be included in the 12.0.0 release? @wjones127

wjones127 · 2023-04-13T00:45:20Z

I merged this and then realized I have just missed the window for 12.0.0. @raulcd would it be possible to add this to the release branch? Otherwise I'll put up a PR to adjust the text of the deprecation warning added in this PR (which says a method was deprecated in 12.0.0).

raulcd · 2023-04-13T09:10:42Z

@raulcd would it be possible to add this to the release branch?

I will add it to 12.0.0

ursabot · 2023-04-15T18:43:41Z

Benchmark runs are scheduled for baseline = 4963105 and contender = 3bd57e3. 3bd57e3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️20.92% ⬆️1.53%] ursa-i9-9960x
[Finished ⬇️1.11% ⬆️0.12%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 3bd57e35 ec2-t3-xlarge-us-east-2
[Failed] 3bd57e35 test-mac-arm
[Finished] 3bd57e35 ursa-i9-9960x
[Finished] 3bd57e35 ursa-thinkcentre-m75q
[Finished] 49631057 ec2-t3-xlarge-us-east-2
[Failed] 49631057 test-mac-arm
[Finished] 49631057 ursa-i9-9960x
[Finished] 49631057 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot · 2023-04-15T18:44:24Z

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

### Rationale for this change Parquet specs support storing key-value metadata provided by the user. However, the parquet-cpp writer can only set it via ParquetFileWriter::Open(). Sometimes user may want to add extra information to it while writing. So it is good to support adding extra key-value metadata any time before closing the file writer. ### What changes are included in this PR? Add a new interface `void AddKeyValueMetadata(std::shared_ptr<const KeyValueMetadata> key_value_metadata)` to the `ParquetFileWriter` class. User can now add more key-value metadata to the file if not closed. ### Are these changes tested? Added a new `Metadata.TestAddKeyValueMetadata` test to verify key-value metadata added before closing the writer are well preserved. ### Are there any user-facing changes? Yes, user can add custom key-value metadata whenever writer is not closed. * Closes: #34888 Lead-authored-by: Gang Wu <[email protected]> Co-authored-by: Will Jones <[email protected]> Signed-off-by: Will Jones <[email protected]>

…pache#34889) ### Rationale for this change Parquet specs support storing key-value metadata provided by the user. However, the parquet-cpp writer can only set it via ParquetFileWriter::Open(). Sometimes user may want to add extra information to it while writing. So it is good to support adding extra key-value metadata any time before closing the file writer. ### What changes are included in this PR? Add a new interface `void AddKeyValueMetadata(std::shared_ptr<const KeyValueMetadata> key_value_metadata)` to the `ParquetFileWriter` class. User can now add more key-value metadata to the file if not closed. ### Are these changes tested? Added a new `Metadata.TestAddKeyValueMetadata` test to verify key-value metadata added before closing the writer are well preserved. ### Are there any user-facing changes? Yes, user can add custom key-value metadata whenever writer is not closed. * Closes: apache#34888 Lead-authored-by: Gang Wu <[email protected]> Co-authored-by: Will Jones <[email protected]> Signed-off-by: Will Jones <[email protected]>

…nd PyArrow (#41633) ### 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? 1. Add `AddKeyValueMetadata` in parquet::arrow 2. Add `add_key_value_metadata` in pyarrow 3. testing ### Are these changes tested? Yes ### Are there any user-facing changes? New api allowing add key-value metadata to Parquet file * GitHub Issue: #41608 Authored-by: mwish <[email protected]> Signed-off-by: mwish <[email protected]>

wgtmac requested a review from wjones127 as a code owner April 4, 2023 15:47

github-actions bot added Component: C++ Component: Parquet awaiting review Awaiting review labels Apr 4, 2023

wgtmac force-pushed the kvmeta branch 2 times, most recently from 1748c1f to c5b322d Compare April 6, 2023 15:51

wjones127 reviewed Apr 7, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 7, 2023

wjones127 reviewed Apr 7, 2023

View reviewed changes

cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved

mapleFU reviewed Apr 8, 2023

View reviewed changes

cpp/src/parquet/file_writer.cc Outdated Show resolved Hide resolved

cpp/src/parquet/file_writer.h Outdated Show resolved Hide resolved

cpp/src/parquet/file_writer.cc Outdated Show resolved Hide resolved

cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 9, 2023

wgtmac requested a review from wjones127 April 10, 2023 01:42

wjones127 requested changes Apr 10, 2023

View reviewed changes

cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved

cpp/src/parquet/metadata.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Apr 10, 2023

wgtmac added 3 commits April 11, 2023 09:36

apacheGH-34888: [C++][Parquet] Writer supports adding extra kv meta

b84556c

address comments

bed0661

make it backward compatible

6f6c41c

wgtmac force-pushed the kvmeta branch from 0970eeb to 6f6c41c Compare April 11, 2023 01:37

try to fix doxygen issue

9b0d022

wgtmac force-pushed the kvmeta branch from 7e4342d to 9b0d022 Compare April 11, 2023 04:49

wgtmac requested a review from wjones127 April 11, 2023 06:05

wjones127 approved these changes Apr 11, 2023

View reviewed changes

cpp/src/parquet/file_writer.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Apr 11, 2023

Update cpp/src/parquet/file_writer.cc

5e5a39c

Co-authored-by: Will Jones <[email protected]>

wgtmac commented Apr 12, 2023

View reviewed changes

wjones127 approved these changes Apr 12, 2023

View reviewed changes

wjones127 merged commit 3bd57e3 into apache:main Apr 13, 2023

mapleFU mentioned this pull request May 24, 2024

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

Merged

	ARROW_DEPRECATED("Deprecated in 12.0.0. Use overload without KeyValueMetadata instead.")
	ARROW_DEPRECATED("Deprecated in 13.0.0. Use overload without KeyValueMetadata instead.")

GH-34888: [C++][Parquet] Writer supports adding extra kv meta #34889

GH-34888: [C++][Parquet] Writer supports adding extra kv meta #34889

Uh oh!

Conversation

wgtmac commented Apr 4, 2023 • edited by github-actions bot 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 Apr 4, 2023

Uh oh!

github-actions bot commented Apr 4, 2023

Uh oh!

wgtmac commented Apr 7, 2023

Uh oh!

wjones127 Apr 7, 2023

Choose a reason for hiding this comment

Uh oh!

wgtmac Apr 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wgtmac commented Apr 10, 2023

Uh oh!

wgtmac commented Apr 11, 2023

Uh oh!

wjones127 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wgtmac Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

wjones127 commented Apr 13, 2023

Uh oh!

raulcd commented Apr 13, 2023

Uh oh!

ursabot commented Apr 15, 2023

Uh oh!

ursabot commented Apr 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wgtmac commented Apr 4, 2023 •

edited by github-actions bot

Loading