Skip to content

Conversation

@corwinjoy
Copy link
Contributor

@corwinjoy corwinjoy commented Sep 2, 2025

Which issue does this PR close?

Rationale for this change

When working with the library using encryption, we have sometimes found it necessary to modify an existing set of WriterProperties on a per-file basis to set specific encryption properties. More generally, others may need to use an existing set of WriterProperties as a template and modify the properties. I have implemented this feature by adding an into_builder method, which appears to be the standard approach in other parts of the library.

Are these changes tested?

Yes, test_writer_properties_builder has been updated to add a round-trip test for into_builder.

Are there any user-facing changes?

Yes. WriterProperties now has a new into_builder method.

@github-actions github-actions bot added the parquet Changes to the parquet crate label Sep 2, 2025
@corwinjoy
Copy link
Contributor Author

@adamreeve
It may also be a good idea to add a Clone property to WriterPropertiesBuilder to facilitate the use of this class as a template for creating WriterProperties.

Copy link
Contributor

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Makes sense to me.

Looking at other into_builder methods in the arrow-rs codebase, they often seem to delegate to calling into or from. It might be nice to also provide an impl From<WriterProperties> for WriterPropertiesBuilder, so users have a choice to use that or the more explicit into_builder.

@corwinjoy
Copy link
Contributor Author

👍 Makes sense to me.

Looking at other into_builder methods in the arrow-rs codebase, they often seem to delegate to calling into or from. It might be nice to also provide an impl From<WriterProperties> for WriterPropertiesBuilder, so users have a choice to use that or the more explicit into_builder.

Alright. I added that as well.

Copy link
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible to me. Thanks @corwinjoy

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @corwinjoy , @adamreeve and @etseidl

I agree this looks good to me

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
@mbrobbel mbrobbel merged commit 9709c09 into apache:main Sep 5, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a way to modify WriterProperties

5 participants