GH-35775: [Go][Parquet] Allow key value file metadata to be written after writing row groups #37786

tschaub · 2023-09-19T02:38:22Z

Rationale for this change

The key value file metadata may include information generated while writing row groups. Currently, it is not possible to set the key value file metadata after creating a writer. With the changes in this branch, key value pairs may be added any time before closing the writer.

What changes are included in this PR?

This branch adds a writer.AppendKeyValueMetadata(key, value) method to the parquet file.Writer and to the pqarrow.FileWriter.

Are these changes tested?

Tests are added for the new functionality.

Are there any user-facing changes?

The KeyValueMetadata field on the parquet file.Writer has been renamed to initialKeyValueMetadata. This is a breaking change. Although the field was exported, setting it did not result in new key value metadata being written. Instead, it represented the initial key value metadata if the writer was passed the WithWriteMetadata write option.

The WithWriteMetadata option can still be used to provide the initial key value metadata values. In addition, the AppendKeyValueMetadata method can be called to add key value pairs after creating a writer.

The FileMetadata field on the parquet file.Writer has been removed. Previously, setting this field value had no effect.

This PR includes breaking changes to public APIs.

The KeyValueMetadata field is no longer exported from the parquet file.Writer struct. Use the WithWriteMetadata writer option to set key value metadata when creating a writer or use the AppendKeyValueMetadata method to add key value metadata after creating a writer.

The FileMetadata field on the parquet file.Writer has been removed.

Closes: [Go][Parquet] pqarrow.FileWriter allow adding parquet metadata after writing rowgroups #35775

github-actions · 2023-09-19T02:38:52Z

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

zeroshade · 2023-09-19T15:34:40Z

go/parquet/file/file_writer.go

+	props                   *parquet.WriterProperties
+	rowGroups               int
+	nrows                   int
+	metadata                metadata.FileMetaDataBuilder
+	fileEncryptor           encryption.FileEncryptor
+	rowGroupWriter          *rowGroupWriter
+	initialKeyValueMetadata metadata.KeyValueMetadata


Since we just initialize the FileMetaDataBuilder with the initialKeyValueMetadata, we don't actually need to store it after we initialize things.

We could probably modify the WriteOption and how the config works by introducing a config struct like type config struct { props *parquet.WriterProperties; initialKVMetadata metadata.KeyValueMetadata } and have WriteOption modify the config struct to initialize the writer. This would let us remove the initialKeyValueMetadata member entirely from the Writer.

It's not necessary (things work just fine currently and I think this is good) but it's an idea.

@zeroshade - I'm happy to make the change to the WriteOption interface if you think that makes sense. I wasn't sure how much of a breaking change would be acceptable.

I could add tschaub@9e5f7c1 to this branch if that sounds good.

I like it, let's add that to this branch

Commit added. It looks like the failing Dev workflow is unrelated.

zeroshade

LGTM

tschaub · 2023-09-21T02:17:19Z

Thanks, @zeroshade

conbench-apache-arrow · 2023-09-21T11:13:48Z

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

There were no benchmark performance regressions. 🎉

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

…tten after writing row groups (apache#37786) ### Rationale for this change The key value file metadata may include information generated while writing row groups. Currently, it is not possible to set the key value file metadata after creating a writer. With the changes in this branch, key value pairs may be added any time before closing the writer. ### What changes are included in this PR? This branch adds a `writer.AppendKeyValueMetadata(key, value)` method to the parquet `file.Writer` and to the `pqarrow.FileWriter`. ### Are these changes tested? Tests are added for the new functionality. ### Are there any user-facing changes? The `KeyValueMetadata` field on the parquet `file.Writer` has been renamed to `initialKeyValueMetadata`. This is a breaking change. Although the field was exported, setting it did not result in new key value metadata being written. Instead, it represented the initial key value metadata if the writer was passed the `WithWriteMetadata` write option. The `WithWriteMetadata` option can still be used to provide the initial key value metadata values. In addition, the `AppendKeyValueMetadata` method can be called to add key value pairs after creating a writer. The `FileMetadata` field on the parquet `file.Writer` has been removed. Previously, setting this field value had no effect. **This PR includes breaking changes to public APIs.** The `KeyValueMetadata` field is no longer exported from the parquet `file.Writer` struct. Use the `WithWriteMetadata` writer option to set key value metadata when creating a writer or use the `AppendKeyValueMetadata` method to add key value metadata after creating a writer. The `FileMetadata` field on the parquet `file.Writer` has been removed. * Closes: apache#35775 Authored-by: Tim Schaub <tim@planet.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>

tschaub requested a review from zeroshade as a code owner September 19, 2023 02:38

github-actions bot added Component: Go awaiting review Awaiting review labels Sep 19, 2023

tschaub mentioned this pull request Sep 19, 2023

[Go][Parquet] pqarrow.FileWriter allow adding parquet metadata after writing rowgroups #35775

Closed

zeroshade added Breaking Change Includes a breaking change to the API Component: Parquet labels Sep 19, 2023

zeroshade changed the title ~~GH-35775: [Go] Allow key value file metadata to be written after writing row groups~~ GH-35775: [Go][Parquet] Allow key value file metadata to be written after writing row groups Sep 19, 2023

zeroshade reviewed Sep 19, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 19, 2023

tschaub added 2 commits September 20, 2023 14:50

Allow key value file metadata to be written after writing row groups

cc76db4

Config for applying writer options

19d3cd7

tschaub force-pushed the file-metadata branch from f3214b4 to 19d3cd7 Compare September 20, 2023 20:50

github-actions bot added awaiting change review Awaiting change review and removed Component: Parquet awaiting changes Awaiting changes labels Sep 20, 2023

zeroshade approved these changes Sep 21, 2023

View reviewed changes

zeroshade merged commit 6eb08dd into apache:main Sep 21, 2023
22 of 25 checks passed

zeroshade removed the awaiting change review Awaiting change review label Sep 21, 2023

github-actions bot added the awaiting merge Awaiting merge label Sep 21, 2023

tschaub deleted the file-metadata branch September 21, 2023 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-35775: [Go][Parquet] Allow key value file metadata to be written after writing row groups #37786

GH-35775: [Go][Parquet] Allow key value file metadata to be written after writing row groups #37786

tschaub commented Sep 19, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Sep 19, 2023

zeroshade Sep 19, 2023

tschaub Sep 20, 2023 •

edited

Loading

zeroshade Sep 20, 2023

tschaub Sep 20, 2023

zeroshade left a comment

tschaub commented Sep 21, 2023

conbench-apache-arrow bot commented Sep 21, 2023

GH-35775: [Go][Parquet] Allow key value file metadata to be written after writing row groups #37786

GH-35775: [Go][Parquet] Allow key value file metadata to be written after writing row groups #37786

Conversation

tschaub commented Sep 19, 2023 • edited by github-actions bot Loading

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 Sep 19, 2023

zeroshade Sep 19, 2023

Choose a reason for hiding this comment

tschaub Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

zeroshade Sep 20, 2023

Choose a reason for hiding this comment

tschaub Sep 20, 2023

Choose a reason for hiding this comment

zeroshade left a comment

Choose a reason for hiding this comment

tschaub commented Sep 21, 2023

conbench-apache-arrow bot commented Sep 21, 2023

tschaub commented Sep 19, 2023 •

edited by github-actions bot

Loading

tschaub Sep 20, 2023 •

edited

Loading