Repository navigation
fix(parquet): preserve CTR algorithm in plaintext footers - #1412
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
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.
|
Moved the regression into
|
### Rationale for this change The encryption configuration suite omits two encrypted columns with AES_GCM_CTR_V1 and a plaintext footer. The shared C++ fixture added in apache/parquet-testing#130 covers this combination and was generated using a branch containing apache/arrow#54884. This follows the writer correction in #1412 with a reusable interoperability regression. Adam Reeve authored the shared fixture. ### What changes are included in this PR? * Add the missing writer configuration for DataPageV1/V2 and Snappy/uncompressed output. * Read generated Go files and the shared C++ fixture through the existing key-retrieval, explicit-key, AAD-mismatch and plaintext-only checks, including values and page indexes. * Fast-forward the parquet-testing submodule from 92d45b0 to eedfad3, which includes the fixture and intervening dataset updates. No production code changes. ### Are these changes tested? * The focused suite passes with `-race -tags assert` and with `-tags assert,noasm`. * Temporarily restoring the old #1412 metadata logic makes all four new generated-file variants fail. The shared C++ fixture's four decryption configurations still pass in that control. The production source was restored afterward. * All 13 test-bearing Parquet packages pass with the race detector; five additional packages have no test files. * Vet and repository-pinned lint pass for `./parquet`; gofmt, diff checks and Apache RAT pass. ### Are there any user-facing changes? No API or runtime behavior changes. AI was used for the tests and PR description.
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.
-race -tags assert. The focused regression also passes withassert,noasm.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.