GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaData by clee704 · Pull Request #41580 · apache/arrow

clee704 · 2024-05-08T01:23:15Z

Rationale for this change

Parquet standard allows reading/writing key-value metadata from/to ColumnChunkMetaData, but there is no way to do that with Parquet C++.

What changes are included in this PR?

Support reading/writing key-value metadata from/to ColumnChunkMetaData with Parquet C++ reader/writer. Support reading key-value metadata from ColumnChunkMetaData with pyarrow.parquet.

Are these changes tested?

Yes, unit tests are added

Are there any user-facing changes?

Yes.

Users can read or write key-value metadata for column chunks with Parquet C++.
Users can read key-value metadata for column chunks with PyArrow.
parquet-reader tool prints key-value metadata in column chunks when --print-key-value-metadata option is used.

GitHub Issue: [C++][Python] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41579

github-actions · 2024-05-08T01:23:37Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2024-05-08T01:26:17Z

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

mapleFU

The idea general LGTM

cc @pitrou @emkornfield @wgtmac

wgtmac · 2024-05-12T15:03:54Z

I saw a related issue: https://issues.apache.org/jira/browse/PARQUET-1648. It seems that parquet-mr does not use it yet.

mapleFU · 2024-05-12T15:05:35Z

It seems that parquet-mr does not use it yet.

Can parquet-mr reads that?

wgtmac · 2024-05-12T15:18:15Z

I don't think so.

https://github.com/apache/parquet-mr/blob/c241170d9bc2cd8415b04e06ecea40ed3d80f64d/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L1596-L1611

mapleFU · 2024-05-12T15:29:33Z

🤔at least this patch is ok and seems other implementation has thrift, it doesn't break the standard..

wgtmac · 2024-05-12T15:30:39Z

I agree. It should not block us from implementing this.

pitrou · 2024-05-13T15:19:03Z

@clee704 Could you please submit a test file to https://github.com/apache/parquet-testing instead of adding it in this PR?

clee704 · 2024-05-29T02:25:33Z

@clee704 Could you please submit a test file to https://github.com/apache/parquet-testing instead of adding it in this PR?

Created apache/parquet-testing#49

mapleFU · 2024-08-04T04:23:39Z

Would check CPython build

 Error compiling Cython file:
------------------------------------------------------------
...
return self.metadata.GetColumnIndexLocation().has_value()
@property
def metadata(self):
"""Additional metadata as key value pairs (dict[bytes, bytes])."""
wrapped = pyarrow_wrap_metadata(self.metadata.key_value_metadata())
^
------------------------------------------------------------
pyarrow/_parquet.pyx:514:18: undeclared name not builtin: pyarrow_wrap_metadata
Error compiling Cython file:
------------------------------------------------------------
...
return self.metadata.GetColumnIndexLocation().has_value()
@property
def metadata(self):
"""Additional metadata as key value pairs (dict[bytes, bytes])."""
wrapped = pyarrow_wrap_metadata(self.metadata.key_value_metadata())
^
------------------------------------------------------------
pyarrow/_parquet.pyx:514:72: Cannot convert 'shared_ptr[const CKeyValueMetadata]' to Python object
[17/136] Compiling Cython CXX source for _dataset...
performance hint: pyarrow/_dataset.pyx:1947:35: Exception check after calling 'GetResultValue[shared_ptr[CRandomAccessFile]]' will always require the GIL to be acquired. Declare 'GetResultValue[shared_ptr[CRandomAccessFile]]' as 'noexcept' if you control the definition and you're sure you don't want the function to raise exceptions.
performance hint: pyarrow/_dataset.pyx:3343:41: Exception check after calling 'GetResultValue[shared_ptr[CRecordBatch]]' will always require the GIL to be acquired. Declare 'GetResultValue[shared_ptr[CRecordBatch]]' as 'noexcept' if you control the definition and you're sure you don't want the function to raise exceptions.
performance hint: pyarrow/_dataset.pyx:3387:34: Exception check after calling 'GetResultValue[CTaggedRecordBatch]' will always require the GIL to be acquired. Declare 'GetResultValue[CTaggedRecordBatch]' as 'noexcept' if you control the definition and you're sure you don't want the function to raise exceptions.
performance hint: pyarrow/_dataset.pyx:3784:42: Exception check after calling 'GetResultValue[CTaggedRecordBatchIterator]' will always require the GIL to be acquired. Declare 'GetResultValue[CTaggedRecordBatchIterator]' as 'noexcept' if you control the definition and you're sure you don't want the function to raise exceptions.
[18/136] Building CXX object CMakeFiles/arrow_python.dir/pyarrow/src/arrow/python/datetime.cc.o

mapleFU · 2024-08-07T07:50:58Z

@wgtmac @pitrou Do you have any more comments here? Lets move forward now

mapleFU · 2024-08-13T16:46:00Z

Would merge this week if no negative comments

wgtmac

+1 on the C++ side

mapleFU · 2024-08-15T13:13:53Z

@pitrou Would you mind take a look at this? I'm planning to merge this pr this week if no negative comments

pitrou · 2024-08-15T14:25:44Z

+
+TEST_F(TestInt32Writer, WriteKeyValueMetadata) {
+ auto writer = this->BuildWriter();
+ writer->AddKeyValueMetadata(KeyValueMetadata::Make({"hello"}, {"world"}));


Can you add a test where AddKeyValueMetadata is called twice?

pitrou · 2024-08-15T14:27:54Z

 }

+template <typename Metadata>
+std::shared_ptr<KeyValueMetadata> CopyKeyValueMetadata(const Metadata& source) {


Perhaps call this FromThriftKeyValueMetadata?

pitrou · 2024-08-15T14:34:46Z

Sorry, I approved while the two comments should probably be addressed before merging this.

mapleFU · 2024-08-15T16:33:05Z

Thanks all, merged!

conbench-apache-arrow · 2024-08-16T03:55:14Z

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

There were no benchmark performance regressions. 🎉

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

jorisvandenbossche · 2024-08-20T08:59:54Z

+@pytest.fixture(scope='module')
+def parquet_test_datadir():
+ result = os.environ.get('PARQUET_TEST_DATA')
+ if not result:
+ raise RuntimeError('Please point the PARQUET_TEST_DATA environment '
+ 'variable to the test data directory')
+ return pathlib.Path(result)


This is the first time we introduce a pyarrow test that requires this to be set up. Do we want to require that strictly, or should we skip the test if the env variable is not set?
In any case this caused a failure in one of the nightly crossbow builds which doesn't have this env variable set (python-emscriptem, https://github.com/ursacomputing/crossbow/actions/runs/10463078918/job/28974494645#step:7:15654)

Both is ok for me but I prefer read that 🤔

…43786) ### Rationale for this change Starting with #41580, the pyarrow tests now also rely on a file in the parquet-testing submodule. And the path to that directory is controlled by `PARQUET_TEST_DATA`, which appears to be set wrongly in the wheel test scripts, causing all wheel builds to fail at the moment. * GitHub Issue: #43785 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

…on emscripten (#43906) ### Rationale for this change The following PR: - #41580 Made mandatory for a test the requirement to have `PARQUET_TEST_DATA` env defined. This is currently not available from `python_test_emscripten.sh` as we require to mount the filesystem for both Node and ChromeDriver. ### What changes are included in this PR? Skip the test that requires `PARQUET_TEST_DATA` for emscripten. ### Are these changes tested? Via archery ### Are there any user-facing changes? No * GitHub Issue: #43905 * GitHub Issue: #43868 Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

…(#43786) ### Rationale for this change Starting with apache/arrow#41580, the pyarrow tests now also rely on a file in the parquet-testing submodule. And the path to that directory is controlled by `PARQUET_TEST_DATA`, which appears to be set wrongly in the wheel test scripts, causing all wheel builds to fail at the moment. * GitHub Issue: #43785 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

…on emscripten (#43906) ### Rationale for this change The following PR: - apache/arrow#41580 Made mandatory for a test the requirement to have `PARQUET_TEST_DATA` env defined. This is currently not available from `python_test_emscripten.sh` as we require to mount the filesystem for both Node and ChromeDriver. ### What changes are included in this PR? Skip the test that requires `PARQUET_TEST_DATA` for emscripten. ### Are these changes tested? Via archery ### Are there any user-facing changes? No * GitHub Issue: #43905 * GitHub Issue: #43868 Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

clee704 added 2 commits May 8, 2024 00:22

Support reading and writing column chunk key-value metadata
9aeb8b1

Python support
408270a

github-actionsBot added Component: Parquet Component: C++ Component: Python awaiting review Awaiting review labels May 8, 2024

clee704 changed the title ~~Support reading/writing key-value metadata from/to ColumnMetaData~~[GH-41579] Support reading/writing key-value metadata from/to ColumnMetaDataMay 8, 2024

clee704 changed the title ~~[GH-41579] Support reading/writing key-value metadata from/to ColumnMetaData~~GH-41579: [C++] Support reading/writing key-value metadata from/to ColumnMetaDataMay 8, 2024

clee704 changed the title ~~GH-41579: [C++] Support reading/writing key-value metadata from/to ColumnMetaData~~GH-41579: [C++][Python] Support reading/writing key-value metadata from/to ColumnMetaDataMay 8, 2024

clee704 changed the title ~~GH-41579: [C++][Python] Support reading/writing key-value metadata from/to ColumnMetaData~~GH-41579: [C++][Python] Support reading/writing key-value metadata from/to ColumnChunkMetaDataMay 8, 2024

clee704 marked this pull request as ready for review May 8, 2024 05:00

clee704 requested a review from wgtmac as a code owner May 8, 2024 05:00

mapleFU changed the title ~~GH-41579: [C++][Python] Support reading/writing key-value metadata from/to ColumnChunkMetaData~~GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaDataMay 8, 2024

mapleFU reviewed May 12, 2024
View reviewed changes

Comment threadcpp/src/parquet/metadata.cc Outdated

github-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 12, 2024

mapleFU requested a review from pitrou May 12, 2024 05:46

mapleFU reviewed May 13, 2024
View reviewed changes

Comment threadcpp/src/parquet/column_writer.cc Outdated

wgtmac requested changes May 13, 2024
View reviewed changes

Comment threadcpp/src/parquet/metadata.cc Outdated
Comment threadcpp/src/parquet/column_writer.h Outdated

pitrou requested changes May 13, 2024
View reviewed changes

clee704 mentioned this pull request May 29, 2024
Add a Parquet file with column chunk key-value metadata apache/parquet-testing#49
Merged

update some non-move impl
1faf669

Trying to fix python tests
25f2a25

mapleFU requested a review from pitrou August 6, 2024 02:32

Merge branch 'main' into columnchunkkeyvaluemetadata
c54926c

wgtmac approved these changes Aug 14, 2024
View reviewed changes

Merge branch 'main' into columnchunkkeyvaluemetadata
96fc780

pitrou approved these changes Aug 15, 2024
View reviewed changes

pitrou self-requested a review August 15, 2024 14:34

Apply suggestions
ab5cb56

mapleFU merged commit 2767dc5 into apache:mainAug 15, 2024

mapleFU removed the awaiting committer review Awaiting committer review label Aug 15, 2024

mapleFU mentioned this pull request Aug 15, 2024
[C++][Python] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41579
Closed

jorisvandenbossche reviewed Aug 20, 2024
View reviewed changes

github-actionsBot added the awaiting changes Awaiting changes label Aug 20, 2024

wgtmac mentioned this pull request Aug 22, 2024
[CI][Python][Parquet] Failed to open local file column_chunk_key_value_metadata.parquet #43785
Closed

jorisvandenbossche mentioned this pull request Aug 22, 2024
GH-43785: [Python][CI] Correct PARQUET_TEST_DATA path in wheel tests #43786
Merged

kou mentioned this pull request Aug 29, 2024
[Python][Parquet][Emscripten] PARQUET_TEST_DATA isn't set in test-conda-python-emscripten #43868
Closed

This was referenced Sep 2, 2024
[CI][Python] python_test_emscripten fails with missing PARQUET_TEST_DATA environment variable #43905
Closed
GH-43868: [CI][Python] Skip test that requires PARQUET_TEST_DATA env on emscripten #43906
Merged

Uh oh!

Conversation

clee704 commented May 8, 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 8, 2024

Uh oh!

github-actionsBot commented May 8, 2024

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wgtmac commented May 12, 2024

Uh oh!

mapleFU commented May 12, 2024

Uh oh!

wgtmac commented May 12, 2024

Uh oh!

mapleFU commented May 12, 2024

Uh oh!

wgtmac commented May 12, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented May 13, 2024

Uh oh!

clee704 commented May 29, 2024

Uh oh!

mapleFU commented Aug 4, 2024

Uh oh!

mapleFU commented Aug 7, 2024

Uh oh!

mapleFU commented Aug 13, 2024

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Aug 15, 2024

Uh oh!

pitrouAug 15, 2024

Choose a reason for hiding this comment

Uh oh!

pitrouAug 15, 2024

Choose a reason for hiding this comment

Uh oh!

pitrou commented Aug 15, 2024

Uh oh!

mapleFU commented Aug 15, 2024

Uh oh!

conbench-apache-arrowBot commented Aug 16, 2024

Uh oh!

jorisvandenbosscheAug 20, 2024

Choose a reason for hiding this comment

Uh oh!

mapleFUAug 20, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

clee704 commented May 8, 2024•
edited
Loading