Repository navigation
GH-3934: Encrypt DataPageV2 repetition and definition levels together with the values - #3935
Draft
RussellSpitzer wants to merge 9 commits into
Draft
RussellSpitzer wants to merge 9 commits into
RussellSpitzer wants to merge 9 commits into
Conversation
…gether with the values The data page module of an encrypted DataPageV2 page is the whole page body. Arrow C++ and arrow-rs encrypt [repetition levels | definition levels | values] as one module, but parquet-java left the levels in plaintext and encrypted only the values, so the two could not read each other's files whenever a page had repetition or definition levels, and the levels leaked the null and list structure of encrypted columns. parquet-java now writes the single module layout, in ColumnChunkPageWriteStore and in ParquetRewriter, through the new ParquetFileWriter.writeEncryptedDataPageV2. The reader recognizes both layouts, so files written by earlier versions can still be read. An encrypted module starts with its own length, so a page body that starts with the body length minus four is a single module; otherwise the levels are in plaintext in front of the module of the values. Tests read files written by Arrow and by the previous parquet-java layout, check what parquet-java and ParquetRewriter write with a reference decoder written from the specification, and check that tampering with levels is detected.
- detect the page layout with both markers, resolve ambiguous GCM pages by the tag check and prefer the legacy layout for unauthenticated CTR pages - validate the level lengths of the page header - replace the public writeEncryptedDataPageV2 by a writeDataPageV2 overload that takes the page block encryptor, so the writer owns the layout - tests: checksums on reads, several row groups, page skipping, a crafted ambiguous legacy page, leak tracking, KMS cache isolation - document the compatibility change in the parquet-hadoop README
…aths; simplify Javadoc
…2 levels in plaintext The writeDataPageV2 overloads that took a page header encryptor but no page block encryptor let callers encrypt only the values, which is the layout of apache#3934. They are removed (with japicmp excludes); the remaining overload rejects a header encryptor without a page block encryptor. The hand-built ambiguous legacy pages are now fixture files.
RussellSpitzer
commented
Nov 6, 2026
RussellSpitzer
commented
Nov 6, 2026
| BytesInput dlLevels = readBlockAllocate(dlLength, reader); | ||
| int payLoadLength = pageHeader.getCompressed_page_size() - rlLength - dlLength; | ||
| int rawDataLength = pageHeader.getUncompressed_page_size() - rlLength - dlLength; | ||
| // Not encrypted here: the writer encrypts the levels and the payload together |
Member
Author
There was a problem hiding this comment.
This is set up so that if you happen to use the rewriter, we will fix the encryption from the old version to the new one.
…ds instead of removing them Restores the four writeDataPageV2 overloads that take only a page header encryptor, deprecated with a warning that they leave the levels of an encrypted column in plaintext, and drops their japicmp excludes. Reads DataPageV2 pages with a switch on the detected layout, notes that the plaintext-levels read path exists for backward compatibility only, and passes the CRC to the fallback page in FullBodyEncryptedDataPageV2.
prtkgaur
reviewed
Nov 6, 2026
prtkgaur
left a comment
prtkgaur
left a comment
There was a problem hiding this comment.
Logic looks correct to me. Will take another pass.
@RussellSpitzer : Do you think the spec should somehow make it clearer to avoid the confusion? "Parquet-java seems to be unique amongst several other implementations in that it encrypts only the values of DataPageV2"
| * @param metadataBlockEncryptor encryptor for the page header | ||
| * @param pageHeaderAAD pageHeader AAD | ||
| * @throws IOException if any I/O error occurs during writing the file | ||
| * @deprecated will be removed in 2.0.0. Use {@link #writeDataPageV2(int, int, int, BytesInput, BytesInput, Encoding, BytesInput, boolean, int, Statistics, BlockCipher.Encryptor, byte[], SizeStatistics, BlockCipher.Encryptor, byte[])} instead |
There was a problem hiding this comment.
not sure what the code standard says, but should this line be wrapped?
Member
Author
There was a problem hiding this comment.
Wrapped in f1a75bc, along with the same line on the other three deprecated overloads. Spotless (palantir) doesn't rewrap Javadoc, and the file has both styles, but these were the only lines over 120 characters in the PR.
A page whose body fits both layouts is now resolved from the writer of the file: parquet-java before 1.19.0 wrote plaintext levels, and later versions and other applications write a single module. Only if created_by is missing, unparseable or a 1.19.0 pre-release does the reader fall back to the GCM tag check, or to plaintext levels for CTR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Parquet-java seems to be unique amongst several other implementations in that it encrypts only the values of DataPageV2 structures separate from repetition and definition levels. All the other implementations (rs, c++, go, duckdb) assume that a DataPageV2 is encrypted as a single module with the the information together.
What changes are included in this PR?
The main change is that all new files will be written encrypting the entire DataPageV2 as a single entity rather than storing levels in plaintext.
Readers are changed to support a fully encrypted DataPageV2 as well as falling back to reading encrypted pages with the previous behavior.
Are these changes tested?
Yes.
Files were generated with both whole module encryption and only encrypting the values. Both are included as test cases
Are there any user-facing changes?
Yes. This is a breaking change for users of Parquet-Java.
Encrypted Files written by the next version of Parquet Java (currently 1.9.0) will be unable to be read by Parquet 1.8 and earlier. They will though be readable by all the other Parquet implementations. Encrypted files written by 1.8 and earlier will remain readable by 1.9.
Closes #3934
This pr was written with the assistance of Claude Opus 5.5 and I haven't fully finished my review which is why I'm leaving this as a draft.