[Parquet] Provide only encrypted column stats in plaintext footer by rok · Pull Request #8305 · apache/arrow-rs

rok · 2025-09-10T10:03:42Z

Closes [Parquet] Provide only encrypted column stats in plaintext footer #8304.

Rationale for this change

See #8304 and especially #8304 (comment)

adamreeve

Looks good thanks Rok, I think this can just be tidied up a bit to reduce some duplication

parquet/src/file/metadata/thrift/mod.rs

parquet/src/file/metadata/writer.rs

parquet/tests/encryption/encryption.rs

adamreeve · 2025-11-20T09:57:20Z

parquet/src/file/metadata/writer.rs

+                let is_footer_encrypted = file_encryptor.properties().encrypt_footer();
+
+                if !is_footer_encrypted {
+                    // Temporarily clear crypto_metadata so statistics get included in encrypted blob


We don't need this step if we check for encrypted_column_metadata instead of crypto_metadata in serialize_column_meta_data (https://github.com/apache/arrow-rs/pull/8305/files#r2543948275)

Agreed and removed.

rok · 2025-12-03T04:23:21Z

Thanks for the review @adamreeve and nice test suggestions, more explicitly presented cases help reason about this.
I think I covered all comments and this is ready for another round of review.

adamreeve

Nice, thanks Rok! Just one minor comment

parquet/src/file/metadata/thrift/mod.rs

rok · 2025-12-03T09:07:19Z

@etseidl would you have time to give this a quick look before we merge?

etseidl

Thanks @rok and @adamreeve, this looks good to me. Just one question (because I'm too lazy to read the spec >.<).

etseidl · 2025-12-03T17:07:08Z

parquet/src/file/metadata/thrift/mod.rs

-    if let Some(page_encoding_stats) = column_chunk.page_encoding_stats() {
-        last_field_id = page_encoding_stats.write_thrift_field(w, 13, last_field_id)?;
+
+    if should_write_column_stats(column_chunk) {


Is the idea here to only write the required bits of the column meta_data so readers won't complain? If so, then should all of the fields below also be skipped?

Good point, I agree all the below fields should be skipped too.

I checked what the C++ implementation does and it's actually a bit different between the plaintext footer case and when the footer is encrypted but the column is encrypted with a different key: https://github.com/apache/arrow/blob/cbd36b817fc77812f8df1a15bf24314de3b27f29/cpp/src/parquet/metadata.cc#L1748-L1755.

When there's a plaintext footer, only the statistics and encoding_stats are stripped out of the unencrypted metadata, similar to what we're proposing here. But when the footer is also encrypted, it's assumed that the reader can handle when the whole metadata field isn't set so this is completely skipped, which is what was done before.

It might make sense to match what C++ does and keep the previous behaviour of excluding the full ColumnChunk metadata field when the footer is encrypted, and check that the reader can handle this when it doesn't have the column key. That should have the benefit of not increasing the footer size too much. And then in the plaintext footer case, also exclude the other fields below.

Good catch @etseidl !
I've moved the other sensitive metadata into the conditional and added tests. (except for geospatial since it requires #[cfg(feature = "geospatial")] and it seems straightforward enough to not cover here?)

Thanks @rok, looks good now. I'll leave it to @adamreeve if we want to revert to excluding the meta_data entirely when the footer is encrypted.

Per @adamreeve suggestion this now omits metadata in case encrypted metadata is present and strips out stats if it is not.

adamreeve · 2025-12-17T00:45:16Z

parquet/src/file/metadata/writer.rs

+                // We always encrypt column metadata separately and store in
+                // encrypted_column_metadata. This allows skipping the plaintext meta_data
+                // field in the writer to reduce footer size. If encrypted_column_metadata
+                // were not set, the reader would not be able to read the column metadata.
+                Some(file_encryptor.get_footer_encryptor()?)


I don't think this is the right approach. If the footer is already encrypted we shouldn't need to double-encrypt the column metadata. This goes against what the spec describes and might be incompatible with some readers, plus it will require extra decryption time when reading.

If it's too complex to wire through the information about whether the footer is a plaintext or not to serialize_column_meta_data, I think we should stick with what you had before, or go back to what we currently do in the main branch where the full column metadata is skipped rather than just the stats. My understanding is that writing the metadata in plaintext without the stats was for backwards compatibility with readers that expect the column metadata to always be present when encryption was first being added. But that might not be such a concern now if readers know that field is optional.

Sorry, forgot to reply here. I've introduced bool ColumnChunkMeteData.plaintext_footer_mode that now helps writer decide if it should write metadata or not. See change.

adamreeve

This all looks good to me now thanks Rok, and it's nice that we match what the C++ library does.

Co-authored-by: Adam Reeve <[email protected]>

rok · 2026-01-07T14:43:16Z

Happy new year @etseidl !
I think we're happy with this now, feel free to do another review when you have time.

etseidl

Happy New Year @rok! Looks good to me.

alamb · 2026-01-17T16:26:42Z

Merged up to resolve a conflict

rok · 2026-01-21T15:55:58Z

Tests required setting ArrowReaderOptions.with_encoding_stats_as_mask(false) to pass again.

etseidl

Change to the test is fine. Alternatively you could leave the defaults and change to testing if the encoding stats mask is Some. Not worth waiting any longer, however.

alamb · 2026-01-23T17:46:43Z

Shall we merge this one in?

rok · 2026-01-23T17:47:38Z

Yes please :)

…ache#8305) - Closes apache#8304. See apache#8304 and especially apache#8304 (comment) --------- Co-authored-by: Adam Reeve <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

alamb · 2026-01-30T15:06:37Z

Backport PR:

[57_maintenance[Parquet] Provide only encrypted column stats in plaintext footer (#8305) #9310

@rok

…text footer (#8305) (#9310) - Part of #9240 - Related to #8304 This is a backport of the following PR to the 57 line - #8305 from @rok Co-authored-by: Rok Mihevc <[email protected]> Co-authored-by: Adam Reeve <[email protected]>

github-actions bot added the parquet Changes to the parquet crate label Sep 10, 2025

rok force-pushed the plaintext_statistics branch from 241c532 to a5a44f4 Compare September 23, 2025 19:50

rok force-pushed the plaintext_statistics branch 2 times, most recently from 3a7de46 to e2ebc45 Compare October 28, 2025 09:14

rok force-pushed the plaintext_statistics branch 2 times, most recently from 03e52aa to 5459ffc Compare November 4, 2025 19:54

rok force-pushed the plaintext_statistics branch 2 times, most recently from 078b622 to 686b38f Compare November 12, 2025 00:44

rok marked this pull request as ready for review November 19, 2025 14:11

rok requested a review from adamreeve November 19, 2025 16:37

adamreeve reviewed Nov 19, 2025

View reviewed changes

adamreeve reviewed Nov 20, 2025

View reviewed changes

adamreeve approved these changes Dec 3, 2025

View reviewed changes

parquet/src/file/metadata/thrift/mod.rs Outdated Show resolved Hide resolved

rok requested a review from etseidl December 3, 2025 09:07

etseidl approved these changes Dec 3, 2025

View reviewed changes

adamreeve requested changes Dec 17, 2025

View reviewed changes

rok force-pushed the plaintext_statistics branch from a6a3ec2 to 5e195dc Compare December 23, 2025 21:09

adamreeve approved these changes Jan 7, 2026

View reviewed changes

rok added 10 commits January 7, 2026 15:14

Initial commit

5842cdb

rebase

51cc3ba

rebase and adjust to refactor

6ca47ce

tmp

936935c

Remove statistics just prior to thrift serialization

28dfb3a

minor change

fb8cb39

test writer produced metadata

d895be7

DRY

b9b0786

not there yet

65c473d

work

d799d9f

adamreeve and others added 10 commits January 7, 2026 15:14

Clippy fix

ef2cedb

review feedback

93b1a98

clippy

91f23f1

work

dd55539

Apply suggestion from @adamreeve

1b74fb1

Co-authored-by: Adam Reeve <[email protected]>

review feedback

11738ce

Review feedback

ca315df

Review feedback

e14520e

lint

688f7c6

clarifying comment

2bf1e29

rok force-pushed the plaintext_statistics branch from bb283d4 to 2bf1e29 Compare January 7, 2026 14:35

rok requested a review from etseidl January 7, 2026 14:42

etseidl approved these changes Jan 7, 2026

View reviewed changes

Merge remote-tracking branch 'apache/main' into plaintext_statistics

237e04e

set ArrowReaderOptions.with_encoding_stats_as_mask(false)

f434c86

rok requested review from adamreeve and etseidl January 21, 2026 15:55

adamreeve approved these changes Jan 21, 2026

View reviewed changes

etseidl approved these changes Jan 22, 2026

View reviewed changes

etseidl mentioned this pull request Jan 22, 2026

Release arrow-rs / parquet Minor version 57.3.0 (January 2026) #9240

Closed

10 tasks

alamb merged commit 7034a63 into apache:main Jan 23, 2026
16 checks passed

alamb mentioned this pull request Jan 30, 2026

[57_maintenance[Parquet] Provide only encrypted column stats in plaintext footer (#8305) #9310

Merged

Comments

Conversation

rok commented Sep 10, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rok commented Dec 3, 2025

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rok commented Dec 3, 2025

Uh oh!

etseidl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamreeve Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

rok commented Jan 7, 2026

Uh oh!

etseidl left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jan 17, 2026

Uh oh!

rok commented Jan 21, 2026

Uh oh!

etseidl left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Jan 23, 2026

Uh oh!

rok commented Jan 23, 2026

Uh oh!

Uh oh!

alamb commented Jan 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rok commented Sep 10, 2025 •

edited by alamb

Loading

adamreeve Dec 3, 2025 •

edited

Loading