[GitHub] [arrow] pitrou commented on a diff in pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files

[GitBox <gi...@apache.org>]

pitrou commented on code in PR #12778:
URL: https://github.com/apache/arrow/pull/12778#discussion_r850475720


##########
cpp/src/parquet/encryption/internal_file_decryptor.h:
##########
@@ -97,12 +97,7 @@ class InternalFileDecryptor {
   std::shared_ptr<Decryptor> footer_data_decryptor_;
   ParquetCipher::type algorithm_;
   std::string footer_key_metadata_;
-  std::vector<encryption::AesDecryptor*> all_decryptors_;
-
-  /// Key must be 16, 24 or 32 bytes in length. Thus there could be up to three
-  // types of meta_decryptors and data_decryptors.
-  std::unique_ptr<encryption::AesDecryptor> meta_decryptor_[3];
-  std::unique_ptr<encryption::AesDecryptor> data_decryptor_[3];
+  std::vector<std::weak_ptr<encryption::AesDecryptor>> all_decryptors_;

Review Comment:
   Can you add a comment explaining the purpose of this attribute?



##########
cpp/src/parquet/encryption/encryption_internal.h:
##########
@@ -88,8 +88,9 @@ class AesDecryptor {
   explicit AesDecryptor(ParquetCipher::type alg_id, int key_len, bool metadata,
                         bool contains_length = true);
 
-  static AesDecryptor* Make(ParquetCipher::type alg_id, int key_len, bool metadata,
-                            std::vector<AesDecryptor*>* all_decryptors);
+  static std::shared_ptr<AesDecryptor> Make(

Review Comment:
   Can you add a docstring here, especially explaining the use of `all_decryptors`?



##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -134,8 +136,11 @@ std::shared_ptr<Decryptor> InternalFileDecryptor::GetFooterDecryptor(
 
   // Create both data and metadata decryptors to avoid redundant retrieval of key
   // from the key_retriever.
-  auto aes_metadata_decryptor = GetMetaAesDecryptor(footer_key.size());
-  auto aes_data_decryptor = GetDataAesDecryptor(footer_key.size());
+  int key_len = static_cast<int>(footer_key.size());
+  auto aes_metadata_decryptor =
+      encryption::AesDecryptor::Make(algorithm_, key_len, true, &all_decryptors_);
+  auto aes_data_decryptor =
+      encryption::AesDecryptor::Make(algorithm_, key_len, false, &all_decryptors_);

Review Comment:
   Make argument meaning more explicit:
   ```suggestion
     auto aes_metadata_decryptor =
         encryption::AesDecryptor::Make(algorithm_, key_len, /*metadata=*/true, &all_decryptors_);
     auto aes_data_decryptor =
         encryption::AesDecryptor::Make(algorithm_, key_len, /*metadata=*/false, &all_decryptors_);
   ```



##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -134,8 +136,11 @@ std::shared_ptr<Decryptor> InternalFileDecryptor::GetFooterDecryptor(
 
   // Create both data and metadata decryptors to avoid redundant retrieval of key
   // from the key_retriever.
-  auto aes_metadata_decryptor = GetMetaAesDecryptor(footer_key.size());
-  auto aes_data_decryptor = GetDataAesDecryptor(footer_key.size());
+  int key_len = static_cast<int>(footer_key.size());
+  auto aes_metadata_decryptor =
+      encryption::AesDecryptor::Make(algorithm_, key_len, true, &all_decryptors_);
+  auto aes_data_decryptor =
+      encryption::AesDecryptor::Make(algorithm_, key_len, false, &all_decryptors_);

Review Comment:
   (you'll need to reformat, sorry :-))



##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -195,8 +200,11 @@ std::shared_ptr<Decryptor> InternalFileDecryptor::GetColumnDecryptor(
 
   // Create both data and metadata decryptors to avoid redundant retrieval of key
   // using the key_retriever.
-  auto aes_metadata_decryptor = GetMetaAesDecryptor(column_key.size());
-  auto aes_data_decryptor = GetDataAesDecryptor(column_key.size());
+  int key_len = static_cast<int>(column_key.size());
+  auto aes_metadata_decryptor =
+      encryption::AesDecryptor::Make(algorithm_, key_len, true, &all_decryptors_);

Review Comment:
   Same here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org