GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41580

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

mapleFU

The idea general LGTM

cc @pitrou @emkornfield @wgtmac

mapleFU · 2024-05-12T05:40:52Z

cpp/src/parquet/metadata.cc

+  KeyValueMetadata& key_value_metadata() {
+    if (!key_value_metadata_) {
+      key_value_metadata_ = std::make_unique<KeyValueMetadata>();
+    }
+    return *key_value_metadata_;


So once key_value_metadata() is called, the metadata is generated, and the metadata would be put in SetKeyValueMetadata?

Should we just add a "Put" here?

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.

mapleFU · 2024-05-13T05:24:34Z

cpp/src/parquet/column_writer.cc

@@ -1405,6 +1405,10 @@ class TypedColumnWriterImpl : public ColumnWriterImpl, public TypedColumnWriter<
    return pages_change_on_record_boundaries_;
  }

+  KeyValueMetadata& key_value_metadata() override {
+    return metadata_->key_value_metadata();


Should we also check !close_ here?

wgtmac · 2024-05-13T05:34:05Z

cpp/src/parquet/metadata.cc

+    kv_pair.__set_value(source.value(i));
+    metadata.key_value_metadata.push_back(kv_pair);
+  }
+  metadata.__isset.key_value_metadata = true;


Can we use void __set_key_value_metadata(const std::vector<KeyValue> & val); instead?

wgtmac · 2024-05-13T05:41:14Z

cpp/src/parquet/column_writer.h

@@ -181,6 +184,10 @@ class PARQUET_EXPORT ColumnWriter {
  /// \brief The file-level writer properties
  virtual const WriterProperties* properties() = 0;

+  /// \brief Return KeyValueMetadata that can be used to store key-value
+  /// metadata in ColumnChunkMetaData.
+  virtual KeyValueMetadata& key_value_metadata() = 0;


I'd rather not to give the freedom to get the mutable object. Instead, we can implement a similar function equivalent to https://github.com/apache/arrow/blob/main/cpp/src/parquet/file_writer.h#L163

pitrou · 2024-05-13T15:07:41Z

cpp/src/parquet/column_writer.h

 class Array;
+class KeyValueMetadata;


Can you please remove this and instead include arrow/type_fwd.h?

pitrou · 2024-05-13T15:08:04Z

cpp/src/parquet/column_writer.h

@@ -53,6 +54,8 @@ class Encryptor;
 class OffsetIndexBuilder;
 class WriterProperties;

+using KeyValueMetadata = ::arrow::KeyValueMetadata;


It's generally better to avoid using directives in non-internal .h files.

pitrou · 2024-05-13T15:08:27Z

cpp/src/parquet/column_writer.h

@@ -181,6 +184,10 @@ class PARQUET_EXPORT ColumnWriter {
  /// \brief The file-level writer properties
  virtual const WriterProperties* properties() = 0;

+  /// \brief Return KeyValueMetadata that can be used to store key-value
+  /// metadata in ColumnChunkMetaData.
+  virtual KeyValueMetadata& key_value_metadata() = 0;


pitrou · 2024-05-13T15:10:58Z

cpp/src/parquet/metadata.h

  // column metadata
  void SetStatistics(const EncodedStatistics& stats);
+
+  KeyValueMetadata& key_value_metadata();


Please let's avoid non-const references, as per our C++ conventions. Instead, you could follow the current builder pattern:

Suggested change

KeyValueMetadata& key_value_metadata();

void SetKeyValueMetadata(const std::shared_ptr<const KeyValueMetadata>&);

pitrou · 2024-05-13T15:12:29Z

cpp/src/parquet/printer.cc

+
+void PrintKeyValueMetadata(std::ostream& stream,
+                           const KeyValueMetadata& key_value_metadata,
+                           int indent_level = 0, int indent_width = 1) {


Seems like it would be easier to just pass the indentation string?

Suggested change

int indent_level = 0, int indent_width = 1) {

std::string_view indent = "") {

pitrou · 2024-05-13T15:16:26Z

python/pyarrow/_parquet.pyx

+        cdef:
+            unordered_map[c_string, c_string] metadata
+            const CKeyValueMetadata* underlying_metadata
+        underlying_metadata = self.metadata.key_value_metadata().get()
+        if underlying_metadata != NULL:
+            underlying_metadata.ToUnorderedMap(&metadata)
+            return metadata
+        else:
+            return None


Should use pyarrow_wrap_metadata instead. Something like (untested):

Suggested change

cdef:

unordered_map[c_string, c_string] metadata

const CKeyValueMetadata* underlying_metadata

underlying_metadata = self.metadata.key_value_metadata().get()

if underlying_metadata != NULL:

underlying_metadata.ToUnorderedMap(&metadata)

return metadata

else:

return None

wrapped = pyarrow_wrap_metadata(self.metadata.key_value_metadata())

if wrapped is not None:

return wrapped.to_dict()

else:

return wrapped

pitrou · 2024-05-13T15:17:26Z

python/pyarrow/tests/parquet/test_metadata.py

+def test_column_chunk_key_value_metadata(datadir):
+    metadata = pq.read_metadata(datadir / 'column-chunk-key-value-metadata.parquet')
+    key_value_metadata = metadata.row_group(0).column(0).metadata
+    print(key_value_metadata)


Can you please remove this print() call (which I assume was meant for debugging)?

pitrou · 2024-05-13T15:17:48Z

python/pyarrow/tests/parquet/test_metadata.py

+    metadata = pq.read_metadata(datadir / 'column-chunk-key-value-metadata.parquet')
+    key_value_metadata = metadata.row_group(0).column(0).metadata
+    print(key_value_metadata)
+    assert key_value_metadata[b'foo'] == b'bar'


Can you test the entire dict contents?

Can you add a test where the metadata is None?

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 added 2 commits May 8, 2024 00:22

Support reading and writing column chunk key-value metadata

9aeb8b1

Python support

408270a

github-actions bot 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 ColumnMetaData May 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 ColumnMetaData May 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 ColumnMetaData May 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 ColumnChunkMetaData May 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 ColumnChunkMetaData May 8, 2024

mapleFU reviewed May 12, 2024

View reviewed changes

github-actions bot 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

wgtmac requested changes May 13, 2024

View reviewed changes

pitrou requested changes May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41580

GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41580

clee704 commented May 8, 2024 •

edited

github-actions bot commented May 8, 2024

github-actions bot commented May 8, 2024

mapleFU left a comment

mapleFU May 12, 2024

wgtmac commented May 12, 2024

mapleFU commented May 12, 2024

wgtmac commented May 12, 2024

mapleFU commented May 12, 2024

wgtmac commented May 12, 2024

mapleFU May 13, 2024

wgtmac May 13, 2024

wgtmac May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou May 13, 2024

pitrou commented May 13, 2024

	KeyValueMetadata& key_value_metadata();
	void SetKeyValueMetadata(const std::shared_ptr<const KeyValueMetadata>&);

	int indent_level = 0, int indent_width = 1) {
	std::string_view indent = "") {

GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41580

Are you sure you want to change the base?

GH-41579: [C++][Python][Parquet] Support reading/writing key-value metadata from/to ColumnChunkMetaData #41580

Conversation

clee704 commented May 8, 2024 • edited

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 8, 2024

github-actions bot commented May 8, 2024

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac commented May 12, 2024

mapleFU commented May 12, 2024

wgtmac commented May 12, 2024

mapleFU commented May 12, 2024

wgtmac commented May 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented May 13, 2024

clee704 commented May 8, 2024 •

edited