You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/03 22:42:59 UTC
[GitHub] [arrow] andersonm-ibm opened a new pull request, #12778: [ARROW-14114] Fix multi-threaded read of PME files
andersonm-ibm opened a new pull request, #12778:
URL: https://github.com/apache/arrow/pull/12778
Change AesDecryptor to be per Decryptor, instead of shared.
This solves the problem of reading with PME using multiple threads.
Was discovered when exposing high-level PME in PyArrow.
Co-authored-by: Gidon Gershinsky <gg...@apple.com>
CC @thamht4190 @pitrou @revit13
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1117053728
Revision: fa7944729ff97f8261ae8bb44a7cfcf28c1c8c40
Submitted crossbow builds: [ursacomputing/crossbow @ actions-2007](https://github.com/ursacomputing/crossbow/branches/all?query=actions-2007)
|Task|Status|
|----|------|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-build-cpp-fuzz)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2007-azure-test-conda-cpp-valgrind)|
|test-conda-python-3.10|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.10)|
|test-conda-python-3.7|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7)|
|test-conda-python-3.7-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-hdfs-2.9.2)|
|test-conda-python-3.7-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-hdfs-3.2.1)|
|test-conda-python-3.7-kartothek-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-kartothek-latest)|
|test-conda-python-3.7-kartothek-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-kartothek-master)|
|test-conda-python-3.7-pandas-0.24|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-pandas-0.24)|
|test-conda-python-3.7-pandas-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-pandas-latest)|
|test-conda-python-3.7-spark-v3.1.2|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-spark-v3.1.2)|
|test-conda-python-3.7-turbodbc-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-turbodbc-latest)|
|test-conda-python-3.7-turbodbc-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.7-turbodbc-master)|
|test-conda-python-3.8|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.8)|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.8-hypothesis)|
|test-conda-python-3.8-pandas-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.8-pandas-latest)|
|test-conda-python-3.8-pandas-nightly|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.8-pandas-nightly)|
|test-conda-python-3.8-spark-v3.2.0|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.8-spark-v3.2.0)|
|test-conda-python-3.9|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.9)|
|test-conda-python-3.9-dask-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.9-dask-latest)|
|test-conda-python-3.9-dask-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.9-dask-master)|
|test-conda-python-3.9-pandas-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.9-pandas-master)|
|test-conda-python-3.9-spark-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-conda-python-3.9-spark-master)|
|test-debian-10-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-debian-10-cpp-amd64)|
|test-debian-10-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-debian-10-cpp-i386)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-debian-11-cpp-amd64)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-debian-11-cpp-i386)|
|test-debian-11-python-3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2007-azure-test-debian-11-python-3)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-fedora-35-cpp)|
|test-fedora-35-python-3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2007-azure-test-fedora-35-python-3)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-20.04-cpp-bundled)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
|test-ubuntu-20.04-python-3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2007-azure-test-ubuntu-20.04-python-3)|
|test-ubuntu-21.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2007-github-test-ubuntu-21.04-cpp)|
--
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
[GitHub] [arrow] pitrou commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1117094427
@andersonm-ibm Can you also rebase on git master?
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by 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
[GitHub] [arrow] github-actions[bot] commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1117096907
Revision: 59e5071f19b5d773b12cadec85193137be2caa5b
Submitted crossbow builds: [ursacomputing/crossbow @ actions-2009](https://github.com/ursacomputing/crossbow/branches/all?query=actions-2009)
|Task|Status|
|----|------|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-build-cpp-fuzz)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2009-azure-test-conda-cpp-valgrind)|
|test-conda-python-3.10|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.10)|
|test-conda-python-3.7|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7)|
|test-conda-python-3.7-hdfs-2.9.2|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-hdfs-2.9.2)|
|test-conda-python-3.7-hdfs-3.2.1|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-hdfs-3.2.1)|
|test-conda-python-3.7-kartothek-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-kartothek-latest)|
|test-conda-python-3.7-kartothek-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-kartothek-master)|
|test-conda-python-3.7-pandas-0.24|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-pandas-0.24)|
|test-conda-python-3.7-pandas-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-pandas-latest)|
|test-conda-python-3.7-spark-v3.1.2|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.7-spark-v3.1.2)|
|test-conda-python-3.8|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.8)|
|test-conda-python-3.8-hypothesis|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.8-hypothesis)|
|test-conda-python-3.8-pandas-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.8-pandas-latest)|
|test-conda-python-3.8-pandas-nightly|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.8-pandas-nightly)|
|test-conda-python-3.8-spark-v3.2.0|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.8-spark-v3.2.0)|
|test-conda-python-3.9|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.9)|
|test-conda-python-3.9-dask-latest|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.9-dask-latest)|
|test-conda-python-3.9-dask-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.9-dask-master)|
|test-conda-python-3.9-pandas-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.9-pandas-master)|
|test-conda-python-3.9-spark-master|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-conda-python-3.9-spark-master)|
|test-debian-10-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-debian-10-cpp-amd64)|
|test-debian-10-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-debian-10-cpp-i386)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-debian-11-cpp-amd64)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-debian-11-cpp-i386)|
|test-debian-11-python-3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2009-azure-test-debian-11-python-3)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-fedora-35-cpp)|
|test-fedora-35-python-3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2009-azure-test-fedora-35-python-3)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-20.04-cpp-bundled)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
|test-ubuntu-20.04-python-3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-2009-azure-test-ubuntu-20.04-python-3)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-2009-github-test-ubuntu-22.04-cpp)|
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #12778:
URL: https://github.com/apache/arrow/pull/12778#discussion_r864567688
##########
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:
It seems you forgot to address this comment?
--
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
[GitHub] [arrow] pitrou commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1099216439
Can you explain what is being accessed from multiple threads in this context?
--
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
[GitHub] [arrow] pitrou commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1117095659
@github-actions crossbow submit -g cpp -g python
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #12778:
URL: https://github.com/apache/arrow/pull/12778#discussion_r864574772
##########
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:
(also the `metadata` argument deserves describing)
--
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
[GitHub] [arrow] ursabot commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1120606660
Benchmark runs are scheduled for baseline = 860ce72cd68a433574fc1ba98fb799e8ce751c2f and contender = 88bccab18a4ab818355209e45862cc52f9cf4a0d. 88bccab18a4ab818355209e45862cc52f9cf4a0d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/bbaf3bcad7094e91b24db022b5d4937e...95335223321d4a83af3b9e7e22a5b210/)
[Finished :arrow_down:0.35% :arrow_up:0.08%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/06211c3a4e5642e2aaa196e0a98aaf54...afaffe6e6a154e0c8e6d784569eaaaea/)
[Finished :arrow_down:0.36% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d495a4468f13497aa0606cf69a82e099...86103ad996cd432483ab083823884f99/)
[Finished :arrow_down:0.99% :arrow_up:0.08%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/c588f3994b11467387d5614923e62edd...7b5dc1544cc14b37b253a3052d29a339/)
Buildkite builds:
[Finished] [`88bccab1` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/704)
[Finished] [`88bccab1` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/701)
[Finished] [`88bccab1` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/690)
[Finished] [`88bccab1` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/706)
[Finished] [`860ce72c` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/703)
[Finished] [`860ce72c` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/700)
[Finished] [`860ce72c` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/689)
[Finished] [`860ce72c` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/705)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] pitrou commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1117052646
@github-actions crossbow submit -g cpp -g python
--
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
[GitHub] [arrow] andersonm-ibm commented on a diff in pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
andersonm-ibm commented on code in PR #12778:
URL: https://github.com/apache/arrow/pull/12778#discussion_r865783148
##########
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:
Sure, @pitrou , I've added the comment.
Thanks for rebasing.
Please, let me know if I can help with anything else..
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1086991617
:warning: Ticket **has not been started in JIRA**, please click 'Start Progress'.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1086991605
https://issues.apache.org/jira/browse/ARROW-14114
--
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
[GitHub] [arrow] pitrou closed pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
URL: https://github.com/apache/arrow/pull/12778
--
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
[GitHub] [arrow] ursabot commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1120606712
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/d495a4468f13497aa0606cf69a82e099...86103ad996cd432483ab083823884f99/)
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12778: [ARROW-14114] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1086964583
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW
Opening JIRAs ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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
[GitHub] [arrow] andersonm-ibm commented on pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
andersonm-ibm commented on PR #12778:
URL: https://github.com/apache/arrow/pull/12778#issuecomment-1105621663
> Can you explain what is being accessed from multiple threads in this context?
`AesDecryptor::AesDecryptorImpl::GcmDecrypt()` and `AesDecryptor::AesDecryptorImpl::CtrDecrypt()` use `ctx_` member of type `EVP_CIPHER_CTX` from OpenSSL, which shouldn't be used from multiple threads concurrently.
So, instead of sharing the same `AesDecryptor` between all `Decryptors`, an `AesDecryptor` will be created per `Decryptor`, which is per column.
--
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
[GitHub] [arrow] andersonm-ibm commented on a diff in pull request #12778: ARROW-14114: [C++][Parquet] Fix multi-threaded read of PME files
Posted by GitBox <gi...@apache.org>.
andersonm-ibm commented on code in PR #12778:
URL: https://github.com/apache/arrow/pull/12778#discussion_r864574139
##########
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:
Sorry, I thought if would be enough to add the description of the `all_decryptors `member.
--
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