Allow adding user defined metadata to `ParquetSink` by wiedld · Pull Request #10224 · apache/datafusion

wiedld · 2024-04-24T23:38:29Z

Which issue does this PR close?

Closes #10223 .

Related to #9493

Rationale for this change

Restore the ability to set user-inserted metadata into the writer properties of ParquetSink, and to enable this as a SQL level API configurable.

What changes are included in this PR?

Broken up per commit:

chore: make explicit what ParquetWriterOptions are created from a subset of TableParquetOptions
- this is code cleanup to make explicit the reported bug
refactor: restore the ability to add kv metadata into the generated file sink
test: demonstrate API contract for metadata TableParquetOptions
- this adds a new SQL level API for adding metadata, including tests.

New feature added UPDATED syntax:

COPY source_table TO 'sink' STORED AS PARQUET OPTIONS ('format.metadata::key' 'value')

Are these changes tested?

Yes. We have both integration tests for the TableParquetOptions API, as well as sqllogictests for the SQL level API.

Are there any user-facing changes?

Yes. The TableParquetOptions API is extended, as well as new SQL level API for these options.

…set of TableParquetOptions

…ile sink

alamb

Thank you @wiedld -- this looks great to me. I had some small documentation and test tweak suggestions, but I also think this PR could be merged as is and we could do them as a follow on PR

Just let me know what you prefer

cc @devinjdangelo and @metesynnada who I think have worked on this code recently

datafusion/core/src/datasource/file_format/parquet.rs

datafusion/common/src/file_options/parquet_writer.rs

datafusion/common/src/config.rs

datafusion/common/src/file_options/parquet_writer.rs

datafusion/sqllogictest/test_files/copy.slt

devinjdangelo

Thank you @wiedld the code looks good to me. The expected syntax of the metadata value isn't 100% clear to me, but this could just be down to the fact I've not worked with parquet kv metadata before.

datafusion/common/src/config.rs

alamb

Looking close

alamb · 2024-04-25T17:33:59Z

datafusion/sqllogictest/test_files/copy.slt

+TO 'test_files/scratch/copy/table_with_metadata/'
+STORED AS PARQUET
+OPTIONS (
+'format.metadata' 'key1:value1 key2:value2'


this is quite and I missed it the first time around -- nice eyes @devinjdangelo . @wiedld could you please add documentation to the TableParquetOptions that documents this behavior?

Specifically, I would be interested to know "what if you want to store metadata values that have spaces in them" (key1:my value with spaces)?

BTW it would be fine if the answer is "you will get an error / is not supported yet" -- it might just be good to document that behavior

I could see wanting to support things like key1:"my awesome value" key2:"my other awesome value" (again not in this PR, but we should at least document how it works I think)

Perhaps we could leverage a syntax like:

(
'format.metadata.key1' 'val1',
'format.metadata.key2' 'val2 with space',
...
)

to support values with spaces

😆 You beat me to it.

I started with the internal double quotes approach, also considered escaped spaces; then I realized that these are introducing lexical rules which did not feel SQL appropriate. Landed on the approach suggested by @devinjdangelo , commit will be up shortly.

… key parsing be a part of the format.metadata::key

wiedld · 2024-04-25T21:49:43Z

datafusion/sqllogictest/test_files/copy.slt

+# accepts multiple entries with the same key (will overwrite)
+statement ok
+COPY source_table
+TO 'test_files/scratch/copy/table_with_metadata/'
+STORED AS PARQUET
+OPTIONS (
+    'format.metadata::key1' 'value',
+    'format.metadata::key1' 'value'
+)


This overwriting is a common feature to all of the config OPTIONS(...). If we want to change, then I recommend a followup ticket.

I agree there is no need to change it in this PR

alamb

Very nice -- thank you for changes @wiedld and thanks for the comments and suggestions @devinjdangelo . I think this PR looks very nice

alamb · 2024-04-25T22:03:05Z

datafusion/common/src/config.rs

+    /// Multiple entries are permitted
+    /// ```sql
+    /// OPTIONS (
+    ///    'format.metadata::key1' '',


alamb · 2024-04-25T22:03:41Z

datafusion/common/src/config.rs

+            .set("format.metadata::key3", "value with spaces ")
+            .unwrap();
+        table_config
+            .set("format.metadata::key4", "value with special chars :: :")


alamb · 2024-04-25T22:04:01Z

datafusion/sqllogictest/test_files/copy.slt

+# accepts multiple entries with the same key (will overwrite)
+statement ok
+COPY source_table
+TO 'test_files/scratch/copy/table_with_metadata/'
+STORED AS PARQUET
+OPTIONS (
+    'format.metadata::key1' 'value',
+    'format.metadata::key1' 'value'
+)


I agree there is no need to change it in this PR

* chore: make explicit what ParquetWriterOptions are created from a subset of TableParquetOptions * refactor: restore the ability to add kv metadata into the generated file sink * test: demomnstrate API contract for metadata TableParquetOptions * chore: update code docs * fix: parse on proper delimiter, and improve tests * fix: enable any character in the metadata string value, by having any key parsing be a part of the format.metadata::key

wiedld added 3 commits April 24, 2024 16:31

chore: make explicit what ParquetWriterOptions are created from a sub…

7b33bc8

…set of TableParquetOptions

refactor: restore the ability to add kv metadata into the generated f…

86bf317

…ile sink

test: demomnstrate API contract for metadata TableParquetOptions

c35b326

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Apr 24, 2024

wiedld mentioned this pull request Apr 24, 2024

ParquetSink: restore ability to provide additional user metadata into the encoded parquet file. #10223

Closed

wiedld changed the title ~~10223/provide metadata to parquet sink~~ fix(10223): provide metadata to parquet sink Apr 24, 2024

wiedld marked this pull request as ready for review April 24, 2024 23:56

alamb mentioned this pull request Apr 25, 2024

DataFusion weekly project plan (Andrew Lamb) - April 22, 2024 #10172

Closed

7 tasks

alamb changed the title ~~fix(10223): provide metadata to parquet sink~~ Allow adding user defined metadata to parquet sink Apr 25, 2024

alamb approved these changes Apr 25, 2024

View reviewed changes

devinjdangelo approved these changes Apr 25, 2024

View reviewed changes

datafusion/common/src/config.rs Outdated Show resolved Hide resolved

wiedld added 2 commits April 25, 2024 09:40

chore: update code docs

cd91a77

fix: parse on proper delimiter, and improve tests

0cf04b5

alamb reviewed Apr 25, 2024

View reviewed changes

wiedld force-pushed the 10223/provide-metadata-to-parquet-sink branch from 6d6e5ba to b7808f1 Compare April 25, 2024 20:34

fix: enable any character in the metadata string value, by having any…

5b06bf3

… key parsing be a part of the format.metadata::key

wiedld force-pushed the 10223/provide-metadata-to-parquet-sink branch from b7808f1 to 5b06bf3 Compare April 25, 2024 20:36

wiedld commented Apr 25, 2024

View reviewed changes

alamb approved these changes Apr 25, 2024

View reviewed changes

alamb changed the title ~~Allow adding user defined metadata to parquet sink~~ Allow adding user defined metadata to ParquetSink Apr 26, 2024

alamb merged commit 9c8873a into apache:main Apr 26, 2024

This was referenced Apr 26, 2024

WIP: apply upstream commit for "enable parallelized writes with ParquetSink" influxdata/arrow-datafusion#13

Closed

WIP(iox-10578): patched df upgrade 202-04-20 influxdata/arrow-datafusion#14

Closed

This was referenced Apr 26, 2024

WIP(iox-10578): patched df upgrade 202-04-23 influxdata/arrow-datafusion#15

Closed

WIP: changes to upstream DF, in order to enable parallelized writes with ParquetSink influxdata/arrow-datafusion#11

Closed

appletreeisyellow mentioned this pull request Apr 30, 2024

WIP: apply upstream commit for "optimizer config param" influxdata/arrow-datafusion#16

Closed

appletreeisyellow mentioned this pull request Apr 30, 2024

WIP(iox-10578): patched df upgrade 202-04-27 influxdata/arrow-datafusion#17

Closed

Comments

Conversation

wiedld commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devinjdangelo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb 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!

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!

alamb 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wiedld commented Apr 24, 2024 •

edited

Loading