[C++][Parquet] "Keep redacted metadata version for old readers"

[pitrou]

Describe the bug, including details regarding any error messages, version, and platform.

This code here looks slightly fishy:

arrow/cpp/src/parquet/metadata.cc

Lines 1787 to 1796 in 8b469b1

	column_chunk_->__set_encrypted_column_metadata(encrypted_column_metadata);
	if (encrypted_footer) {
	column_chunk_->__isset.meta_data = false;
	} else {
	// Keep redacted metadata version for old readers
	column_chunk_->__isset.meta_data = true;
	column_chunk_->meta_data.__isset.statistics = false;
	column_chunk_->meta_data.__isset.encoding_stats = false;
	}

Basically, when encrypting a Parquet file with plaintext footer, the Parquet writer also keeps most of the column metadata in plaintext (except statistics).

Component(s)

C++, Parquet

[pitrou]

@ggershinsky @adamreeve @rok What do you think? Should this fallback "for old readers" be kept?

[ggershinsky]

Yep, maybe this needs to be re-evaluated (together with the spec, https://github.com/apache/parquet-format/blob/master/Encryption.md#55-plaintext-footer-mode), now that ~7 years have passed.
One of the questions, are there users that need to see footers of encrypted files.
Or users that need to access unencrypted columns without having any keys.
A low hanging fruit would be to update the comment "Keep redacted metadata version for old readers" to something like "Keep redacted metadata version for plaintext footer mode ".
A longer-term option is updating the spec ( to either deprecate the plaintext footer mode, or repurpose it to the usecases described above; the latter requires only text/comment updates).

[rok]

It does look shady at first glance, but I think this is just plaintext footer mode.

Regarding deprecating plaintext footer - we should probably have a ML discussion first to get an idea who actually uses it.

[pitrou]

Ah, I had forgotten about this tidbit:

In the plaintext footer mode, the optional ColumnMetaData meta_data is set in the ColumnChunk structure for all columns, but is stripped of the statistics for the sensitive (encrypted) columns.

So this does seem spec-compliant.

[adamreeve]

The spec says that plaintext footer mode "allows legacy Parquet versions (released before the encryption support) to access unencrypted columns in encrypted files." But we (G-Research) like using plaintext footer mode because it makes it easy for engineers to inspect Parquet files and see non-sensitive columns without needing to configure the KMS and decryption settings. So I don't think plaintext footer mode should be removed, and agree with Gidon that we should probably update the spec wording.

If newer readers can handle the missing metadata OK, then maybe the plaintext column metadata for encrypted columns could be completely removed. But we should verify that first, I wouldn't be surprised if that caused problems for readers that do support encryption when they don't have access to those columns, even though it would be spec compliant.