Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` #6840

alamb · 2024-12-05T22:09:27Z

Which issue does this PR close?

Part of Add coerce_types flag to parquet ArrowWriter #1938

Rationale for this change

While working on the upgrade to arrow 54 in DataFusion apache/datafusion#13663 I found it was not clear what the coerce_types argument in arrow_to_parquet_schema meant or what value to pick to keep the existing behavior.

I had to go digging on #6313 to find what the default / old behavior was (true or false)

Turns out the answer is

    let schema_desc = arrow_to_parquet_schema(&schema, false)?;

It is clearly documented on WriterProperties but not on the lower level apis

Thus I would like to propose making a new API for converting schemas where the defaults are clearer and deprecate arrow_to_parquet_schema (and revert the unnecessary breaking API change)

What changes are included in this PR?

Add ArrowToParquetSchemaConverter,
Lots of comments / examples
Revert the changes to arrow_to_parquet_schema made in feat(parquet)!: coerce_types flag for date64 #6313
Deprecate arrow_to_parquet_schema with a clear message about what to use instread

Are there any user-facing changes?

A new API and a deprecation of an existing one

alamb · 2024-12-05T22:10:57Z

parquet/src/arrow/schema/mod.rs

-    arrow_to_parquet_schema_with_root(schema, "arrow_schema", coerce_types)
-}
+#[deprecated(since = "54.0.0", note = "Use `ArrowToParquetSchemaConverter` instead")]
+pub fn arrow_to_parquet_schema(schema: &Schema) -> Result<SchemaDescriptor> {


This reverts the changes made in https://github.com/apache/arrow-rs/pull/6313/files#diff-6a684124e78f254fa6ea23b98f04be0a3ff5a2e4b24890d9c6f4dd850ba11333L232

alamb · 2024-12-05T22:11:42Z

parquet/src/arrow/schema/mod.rs


-/// Convert arrow schema to parquet schema specifying the name of the root schema element
-pub fn arrow_to_parquet_schema_with_root(


It turns out this function is not actually exported (it is pub in this module, but not pub exported):

https://docs.rs/parquet/latest/parquet/?search=arrow_to_parquet_schema_with_root

Returns no results

The compiler told me it was unused once I switched everything over to use ArrowToParquetSchemaConverter

alamb · 2024-12-05T22:13:06Z

parquet/src/file/properties.rs

+    ///
+    /// Setting this option to `true` will result in parquet files that can be
+    /// read by more readers, but may lose precision for arrow types such as
+    /// [`DataType::Date64`] which have no direct corresponding Parquet type.


I also felt it would help if we described more explicitly what the impact of enabling this option was (and left a link to the longer backstory)

…` et al

etseidl · 2024-12-05T22:57:59Z

@alamb could you take a look at #6828? Some of the doc changes will conflict, at least. +1 on the idea, though 😄

etseidl

Big improvement, thanks!

etseidl · 2024-12-05T23:26:07Z

parquet/src/arrow/schema/mod.rs

@@ -225,27 +225,130 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(schema: &Schema, props: &mut
    }
 }

-/// Convert arrow schema to parquet schema
+/// Converter for arrow schema to parquet schema


The docs are pretty inconsistent on whether "arrow" and "parquet" are capitalized or not. Do you have a preference?

I think we should strive for always capitalizing them Arrow and Parquet -- i will fix

parquet/src/arrow/schema/mod.rs

etseidl · 2024-12-05T23:53:33Z

parquet/src/file/properties.rs

+    /// Setting this option to `true` will result in parquet files that can be
+    /// read by more readers, but may lose precision for arrow types such as
+    /// [`DataType::Date64`] which have no direct corresponding Parquet type.


I think we should still mention the naming impacts here. There seem to be a number of users who want Parquet naming, and I think this is the first place they'd look to figure out how to do so. I'm not sure I'd think to follow the link to the converter when I want my lists named properly.

I agree - I pushed e7b7d20 to try and clarify -- let me know what you think

etseidl · 2024-12-05T23:54:55Z

parquet/src/arrow/schema/mod.rs

-        let converted_arrow_schema = arrow_to_parquet_schema(&arrow_schema, true).unwrap();
+        let converted_arrow_schema = ArrowToParquetSchemaConverter::new(&arrow_schema)
+            .with_coerce_types(true)


I love how this makes what's going on more explicit.

Co-authored-by: Ed Seidl <[email protected]>

…to alamb/parquet_coverter

alamb

Thank you for the review @etseidl

parquet/src/arrow/schema/mod.rs

alamb · 2024-12-06T22:20:12Z

parquet/src/file/properties.rs

+    /// Setting this option to `true` will result in parquet files that can be
+    /// read by more readers, but may lose precision for arrow types such as
+    /// [`DataType::Date64`] which have no direct corresponding Parquet type.


I agree - I pushed e7b7d20 to try and clarify -- let me know what you think

etseidl

Just the doc CI failure to fix. Otherwise looks ready to go (mod capitalization which can be a follow on 😉). Thanks again @alamb.

parquet/src/file/properties.rs

Co-authored-by: Ed Seidl <[email protected]>

…to alamb/parquet_coverter

alamb · 2024-12-07T12:22:45Z

Just the doc CI failure to fix. Otherwise looks ready to go (mod capitalization which can be a follow on 😉). Thanks again @alamb.

Thank you 🙏

I fixed a few more capitalization inconsistencies and added a link to the date section of the parquet spec too

tustvold

I did wonder if this should instead be an options struct that gets passed to a method (or has a member function that accepts the actual schema). This would allow using the same config for multiple files, and is perhaps more consistent with other APIs?

parquet/src/arrow/schema/mod.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

…to alamb/parquet_coverter

This reverts commit bd4e2d5.

alamb · 2024-12-16T21:01:24Z

I did wonder if this should instead be an options struct that gets passed to a method (or has a member function that accepts the actual schema). This would allow using the same config for multiple files, and is perhaps more consistent with other APIs?

I agree, as there is no state held in the converter there is no reason for it to be a consuming builder. I made this change in 0ab4eb4

alamb · 2024-12-16T21:32:13Z

Thanks again @etseidl and @tustvold

github-actions bot added the parquet Changes to the parquet crate label Dec 5, 2024

alamb force-pushed the alamb/parquet_coverter branch from b0d96be to 07a754c Compare December 5, 2024 22:09

alamb marked this pull request as draft December 5, 2024 22:10

alamb commented Dec 5, 2024

View reviewed changes

alamb force-pushed the alamb/parquet_coverter branch from 07a754c to d1c78c2 Compare December 5, 2024 22:26

alamb mentioned this pull request Dec 5, 2024

Minor: add example for creating SchemaDescriptor #6841

Merged

alamb force-pushed the alamb/parquet_coverter branch from d1c78c2 to 29d2509 Compare December 5, 2024 22:37

Add ArrowToParquetSchemaConverter, deprecate `arrow_to_parquet_schema…

97d8991

…` et al

alamb force-pushed the alamb/parquet_coverter branch from 29d2509 to 97d8991 Compare December 5, 2024 22:39

Fmt

af74429

alamb mentioned this pull request Dec 5, 2024

Optionally coerce names of maps and lists to match Parquet specification #6828

Merged

alamb added 2 commits December 5, 2024 18:13

Merge remote-tracking branch 'apache/main' into alamb/parquet_coverter

1fe379a

update test

b519da4

etseidl reviewed Dec 6, 2024

View reviewed changes

alamb marked this pull request as ready for review December 6, 2024 14:45

alamb mentioned this pull request Dec 6, 2024

WIP Upgrade to arrow-rs/parquet 54.0.0 apache/datafusion#13663

Open

alamb and others added 6 commits December 6, 2024 17:09

Merge remote-tracking branch 'apache/main' into alamb/parquet_coverter

2296b00

Update parquet/src/arrow/schema/mod.rs

40ce622

Co-authored-by: Ed Seidl <[email protected]>

Apply suggestions from code review

1535b42

Co-authored-by: Ed Seidl <[email protected]>

Improve comments

cec4f8d

Merge branch 'alamb/parquet_coverter' of github.com:alamb/arrow-rs in…

1841283

…to alamb/parquet_coverter

Add more detail to WriterPropertiesBuilder docs

e7b7d20

alamb commented Dec 6, 2024

View reviewed changes

etseidl approved these changes Dec 6, 2024

View reviewed changes

parquet/src/file/properties.rs Outdated Show resolved Hide resolved

alamb and others added 3 commits December 7, 2024 07:20

Update parquet/src/file/properties.rs

4e8c4d6

Co-authored-by: Ed Seidl <[email protected]>

Fix some more capitalization and add a link to Parquet date spec

3ffd4d4

Merge branch 'alamb/parquet_coverter' of github.com:alamb/arrow-rs in…

11ff55e

…to alamb/parquet_coverter

tustvold reviewed Dec 12, 2024

View reviewed changes

parquet/src/arrow/schema/mod.rs Show resolved Hide resolved

parquet/src/arrow/schema/mod.rs Outdated Show resolved Hide resolved

alamb and others added 6 commits December 16, 2024 15:42

Merge remote-tracking branch 'apache/main' into alamb/parquet_coverter

cf6ce05

Update parquet/src/arrow/schema/mod.rs

bd4e2d5

Co-authored-by: Raphael Taylor-Davies <[email protected]>

Merge branch 'alamb/parquet_coverter' of github.com:alamb/arrow-rs in…

afcb179

…to alamb/parquet_coverter

Revert "Update parquet/src/arrow/schema/mod.rs"

17267ec

This reverts commit bd4e2d5.

rename to ArrowSchemaConverter

8892810

change from build --> convert

0ab4eb4

alamb added 2 commits December 16, 2024 16:16

update doc

df3072c

fix fmt

46e57d4

alamb mentioned this pull request Dec 16, 2024

Minor: make it easier to find fix instructions when cargo fmt on parquet fails #6886

Merged

alamb merged commit 2808625 into apache:main Dec 16, 2024
16 checks passed

alamb deleted the alamb/parquet_coverter branch December 16, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` #6840

Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` #6840

alamb commented Dec 5, 2024

alamb Dec 5, 2024

alamb Dec 5, 2024

alamb Dec 5, 2024

etseidl commented Dec 5, 2024

etseidl left a comment

etseidl Dec 5, 2024

alamb Dec 6, 2024

etseidl Dec 5, 2024

alamb Dec 6, 2024

etseidl Dec 6, 2024

etseidl Dec 5, 2024

alamb left a comment

alamb Dec 6, 2024

etseidl left a comment

alamb commented Dec 7, 2024

tustvold left a comment

alamb commented Dec 16, 2024

alamb commented Dec 16, 2024


		/// Convert arrow schema to parquet schema specifying the name of the root schema element
		pub fn arrow_to_parquet_schema_with_root(

Add ArrowToParquetSchemaConverter, deprecate arrow_to_parquet_schema #6840

Add ArrowToParquetSchemaConverter, deprecate arrow_to_parquet_schema #6840

Conversation

alamb commented Dec 5, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etseidl commented Dec 5, 2024

etseidl left a 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

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etseidl left a comment

Choose a reason for hiding this comment

alamb commented Dec 7, 2024

tustvold left a comment

Choose a reason for hiding this comment

alamb commented Dec 16, 2024

alamb commented Dec 16, 2024

Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` #6840

Add `ArrowToParquetSchemaConverter`, deprecate `arrow_to_parquet_schema` #6840