Skip to content

Conversation

@adamreeve
Copy link
Contributor

@adamreeve adamreeve commented Nov 18, 2025

Rationale for this change

Fixes creation of writer properties when writing a Parquet Dataset with encryption, so that column-specific settings aren't lost.

What changes are included in this PR?

Changes the WriterProperties::Builder constructor that uses existing properties so that it also sets any column-specific settings.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, this fixes a user-facing bug.

@adamreeve adamreeve marked this pull request as ready for review November 19, 2025 00:25
content_defined_chunking_options_(
properties.content_defined_chunking_options()) {}
properties.content_defined_chunking_options()) {
for (const auto& [col_path, col_props] : properties.column_properties_) {
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that WriterProperties and WriterProperties::Builder use different forms to represent column properties. Is it possible to simplify this by using the same form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the builder needs to work this way so that different column-specific properties can be changed as well as changing default column properties without the order of operations affecting the result. I don't think the WriterProperties can be changed to use the Builder's representation without breaking the API as ColumnProperties is part of the public API and returned as a reference:

const ColumnProperties& column_properties(

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 20, 2025
Copy link
Member

@rok rok left a comment

Choose a reason for hiding this comment

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

Looks good, strange we don't currently do this. Just a minor change request.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 21, 2025
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Nov 21, 2025
@adamreeve adamreeve merged commit 39c865e into apache:main Nov 25, 2025
38 of 40 checks passed
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 39c865e.

There were no benchmark performance regressions. 🎉

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants