Support Parquet Reader Decryption #101

liushengxuan · 2026-01-09T03:18:58Z

What problem does this PR solve?

Issue Number: close #63

Type of Change

🐛 Bug fix (non-breaking change which fixes an issue)
✨ New feature (non-breaking change which adds functionality)
🚀 Performance improvement (optimization)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🔨 Refactoring (no logic changes)
🔧 Build/CI or Infrastructure changes
📝 Documentation only

Description

Describe your changes in detail.
For complex logic, explain the "Why" and "How".

Performance Impact

No Impact: This change does not affect the critical path (e.g., build system, doc, error handling).

Positive Impact: I have run benchmarks.

Click to view Benchmark Results

Paste your google-benchmark or TPC-H results here.
Before: 10.5s
After:   8.2s  (+20%)

Negative Impact: Explained below (e.g., trade-off for correctness).

Release Note

Please describe the changes in this PR

Release Note:

Release Note:
- Fixed a crash in `substr` when input is null.
- optimized `group by` performance by 20%.

Checklist (For Author)

I have added/updated unit tests (ctest).
I have verified the code with local build (Release/Debug).
I have run clang-format / linters.
(Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes.
No need to test or manual test.

Breaking Changes

No

Yes (Description: ...)

Click to view Breaking Changes

Breaking Changes:
- Description of the breaking change.
- Possible solutions or workarounds.
- Any other relevant information.

This commit introduces the decryption feature for Parquet reader. To use the decryption feature, you need to implement the KMS class for key retrieval.

ZacBlanco

Thanks for these changes @liushengxuan ! This is a large PR, so for this round I only looked over the newly-added code for now. It also seems there is still some CI jobs which failed. I will hold off on another round of review until it is passing, and then review the remaining parts.

Most comments I had are minor, but I had a few higher level ones in there about the structure of the code to support Both CTR and GCM. Let me know what you think

ZacBlanco · 2026-01-22T18:31:37Z

bolt/dwio/common/Options.h

+  std::shared_ptr<::parquet::FileDecryptionProperties::Builder>
+      fileDecryptionPropertiesBuilder_{nullptr};


Does this need to be initialized to nullptr?

And is there a reason this is initialized here rather than the constructor?

ZacBlanco · 2026-01-22T18:34:01Z

bolt/dwio/common/Options.h

+        fileSchema(nullptr) {
+    cryptoFactory_ = std::make_shared<::parquet::encryption::CryptoFactory>();
+  }


Since no args are required we can use the initializer list?

Suggested change

fileSchema(nullptr) {

cryptoFactory_ = std::make_shared<::parquet::encryption::CryptoFactory>();

}

fileSchema(nullptr),

cryptoFactory_(std::make_shared<::parquet::encryption::CryptoFactory>())

{ }

ZacBlanco · 2026-01-22T18:36:38Z

bolt/dwio/common/Options.h

    return *this;
  }

+  // Set default Footer Key for Parquet Decryption. If the Footer Key is empty,


Wondering if setting to empty string is a good idea. I feel like this would be a good place to use std::optional to denote the presence of a key or not. This way we don't need to check for some kind of special value of the string itself.

ZacBlanco · 2026-01-22T18:37:48Z

bolt/dwio/common/Options.h


+  std::shared_ptr<::parquet::FileDecryptionProperties::Builder>
+  getFileDecryptionPropertiesBuilder() const {
+    return fileDecryptionPropertiesBuilder_;


Should we comment that this could potentially be null?

ZacBlanco · 2026-01-22T18:38:06Z

bolt/dwio/common/Options.h

+  ReaderOptions& setFooterDecryptionKey(std::string footerKey) {
+    // fileDecryptionPropertiesBuilder_ is initiated when footer key or column
+    // keys are set directly.
+    if (fileDecryptionPropertiesBuilder_ == nullptr) {


Can we not just initialize the decryptionPropertiesBuilder in the constructor? Why do we lazily initialize it?

ZacBlanco · 2026-01-22T21:52:24Z

bolt/dwio/parquet/decryption/AesDecryptor.h

+  /// \brief Constructor function of AesDecryptor.
+  ///
+  /// \param encryptionType the encryption algorithm to use.
+  /// \param keyLen can only serve one key length. Possible values: 16, 24, 32
+  /// bytes. \param hasMetadataDecryptor if true then this is a metadata
+  /// decryptor. \param containsLength If it is true, expect ciphertext length
+  /// prepended to the ciphertext.
+  explicit AesDecryptor(
+      ParquetCipher::type algId,
+      bool metadata,


This comment doesn't contain documentation on the metadata field

ZacBlanco · 2026-01-22T21:52:51Z

bolt/dwio/parquet/decryption/AesDecryptor.cpp

+  ctx_ = nullptr;
+  lengthBufferLength_ = containsLength ? kBufferSizeLength : 0;
+  ciphertextSizeDelta_ = lengthBufferLength_ + arrow::encryption::kNonceLength;
+  if (metadata || (ParquetCipher::AES_GCM_V1 == algId)) {


Is this the only use of this metadata parameter? Can we not just provide a specialized constructor/factory method which defaults to using AES_GCM_V1 algorithm? Call it something like createAesMetadataDecryptor?

ZacBlanco · 2026-01-23T07:52:59Z

bolt/dwio/parquet/thrift/ThriftInternal.h

+    uint32_t clen = *len;
+    int64_t allocateSize = clen - decryptor->ciphertextSizeDelta();


const? Also why not just use *len directly on L38?

ZacBlanco · 2026-01-23T07:54:06Z

bolt/dwio/parquet/thrift/ThriftInternal.h

+  template <class T>
+  bool DeserializeMessage(
+      const uint8_t* buf,
+      uint32_t* len,


Is this a constant value? Also is there a reason to use uint32_t* instead of uint32_t?

ZacBlanco · 2026-01-23T07:55:21Z

bolt/dwio/parquet/thrift/ThriftInternal.h

+  template <class T>
+  void DeserializeUnencryptedMessage(
+      const uint8_t* buf,
+      uint32_t* len,


similar question on using this pointer as opposed to the value directly

liushengxuan force-pushed the shengxuan_decryption branch from 22496e2 to 886e598 Compare January 21, 2026 16:37

liushengxuan changed the title ~~WIP Introduce Parquet Decryptor~~ Support Paruqet Reader Decryption Jan 21, 2026

liushengxuan force-pushed the shengxuan_decryption branch 4 times, most recently from f5d8c51 to dea8c74 Compare January 22, 2026 15:46

liushengxuan requested review from ZacBlanco, oarap and yangzhg January 22, 2026 15:57

Support Parquet Reader Decryption

7c4ae3a

This commit introduces the decryption feature for Parquet reader. To use the decryption feature, you need to implement the KMS class for key retrieval.

liushengxuan force-pushed the shengxuan_decryption branch from dea8c74 to 7c4ae3a Compare January 22, 2026 17:05

print

e6c7a55

ZacBlanco requested changes Jan 23, 2026

View reviewed changes

ZacBlanco changed the title ~~Support Paruqet Reader Decryption~~ Support Parquet Reader Decryption Jan 23, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Parquet Reader Decryption #101

Support Parquet Reader Decryption #101

Uh oh!

liushengxuan commented Jan 9, 2026 •

edited

Loading

Uh oh!

ZacBlanco left a comment •

edited

Loading

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 22, 2026

Uh oh!

ZacBlanco Jan 23, 2026

Uh oh!

ZacBlanco Jan 23, 2026

Uh oh!

ZacBlanco Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		std::shared_ptr<::parquet::FileDecryptionProperties::Builder>
		fileDecryptionPropertiesBuilder_{nullptr};

		uint32_t clen = *len;
		int64_t allocateSize = clen - decryptor->ciphertextSizeDelta();

Support Parquet Reader Decryption #101

Are you sure you want to change the base?

Support Parquet Reader Decryption #101

Uh oh!

Conversation

liushengxuan commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

Type of Change

Description

Performance Impact

Release Note

Checklist (For Author)

Breaking Changes

Uh oh!

ZacBlanco left a comment • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

liushengxuan commented Jan 9, 2026 •

edited

Loading

ZacBlanco left a comment •

edited

Loading