Skip to content

fix(parquet): preserve CTR algorithm in plaintext footers - #1412

Merged
zeroshade merged 2 commits into
apache:mainfrom
autoCAT6:fix/parquet-plaintext-footer-cipher
Oct 27, 2026
Merged

zeroshade merged 2 commits into
apache:mainfrom
autoCAT6:fix/parquet-plaintext-footer-cipher

Conversation

@autoCAT6

Copy link
Copy Markdown

Rationale for this change

Writing an AES_GCM_CTR_V1 file with a plaintext footer currently records AES_GCM_V1 in the footer metadata. Readers then select GCM for the CTR data pages and fail to read them. This also reproduces with arrow-go v18.8.0.

Related to #566 and apache/arrow#54884.

What changes are included in this PR?

Serialize the configured file algorithm directly, preserving its AAD settings. Footer signing is unchanged.

Add a generated-file regression for both algorithms, encrypted and plaintext footers, and stored or externally supplied AAD prefixes. It checks serialized metadata and round-trip values, plus rejection of a corrupted plaintext-footer signature.

Are these changes tested?

The regression fails on the original code in all three CTR/plaintext cases and passes with the fix.

  • All 18 Parquet packages pass with -race -tags assert. The focused regression also passes with assert,noasm.
  • Files from this commit read correctly with arrow-go v18.8.0, the C++ reader from GH-51669: [C++][Parquet] Preserve cipher in plaintext footer metadata arrow#54884, and parquet-java 1.18.1, including all column values.
  • Vet and lint pass for the changed packages. Apache RAT passes. Broader vet/lint checks report only the existing BitRun benchmark warning and three schema QF1001 suggestions, also reproduced on the unchanged base.

Are there any user-facing changes?

New CTR files with plaintext footers declare the correct cipher and can be read back. No API changes.

AI was used for the implementation, tests and PR description.

@autoCAT6
autoCAT6 requested a review from zeroshade as a code owner October 23, 2026 21:07

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks — a clean fix. Writing encryptProps.Algorithm() directly matches the encrypted-footer path (GetFileCryptoMetaData), and dropping the special-casing of AadPrefix is safe because the properties only carry a prefix when it's stored in the file. The test covers both ciphers across encrypted/plaintext footers and stored/external AAD, and the three CTR plaintext cases fail without the fix. The signature check confirms footer signing still uses GCM.

Minor, non-blocking: this repo prefers folding new tests into an existing test file where one fits, and using memory.NewCheckedAllocator in reader tests.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Arrow handles maintainer review:
CONTRIBUTING.md.

@autoCAT6

Copy link
Copy Markdown
Author

Moved the regression into file_reader_test.go and added a checked allocator to each case, with an assertion that all reader allocations are released after cleanup.

go test -race -tags assert ./parquet/... passes. The focused assert,noasm regression, vet and lint for the affected packages, and RAT also pass.

@zeroshade
zeroshade merged commit d12c428 into apache:main Oct 27, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants