pitrou commented on code in PR #45462:
URL: https://github.com/apache/arrow/pull/45462#discussion_r2534534790
##########
cpp/src/arrow/dataset/file_parquet_encryption_test.cc:
##########
@@ -398,9 +411,191 @@ TEST_P(DatasetEncryptionTest, ReadSingleFile) {
INSTANTIATE_TEST_SUITE_P(DatasetEncryptionTest, DatasetEncryptionTest,
kAllParamValues);
+struct NestedFieldsEncryptionTestParam : EncryptionTestParam {
Review Comment:
Should this use `public` inheritance?
##########
cpp/src/arrow/dataset/file_parquet_encryption_test.cc:
##########
@@ -79,14 +83,20 @@ const auto kAllParamValues = ::testing::Values(
EncryptionTestParam{false, true, true}, EncryptionTestParam{true, true,
true});
// Base class to test writing and reading encrypted dataset.
-class DatasetEncryptionTestBase : public
testing::TestWithParam<EncryptionTestParam> {
+template <typename T, typename = typename std::enable_if<
+ std::is_base_of<EncryptionTestParam,
T>::value>::type>
+class DatasetEncryptionTestBase : public testing::TestWithParam<T> {
public:
#ifdef ARROW_VALGRIND
static constexpr int kConcurrentIterations = 4;
#else
static constexpr int kConcurrentIterations = 20;
#endif
+ static const EncryptionTestParam& GetParam() {
Review Comment:
I'm honestly curious why this is required.
##########
cpp/src/arrow/dataset/file_parquet_encryption_test.cc:
##########
@@ -79,14 +83,20 @@ const auto kAllParamValues = ::testing::Values(
EncryptionTestParam{false, true, true}, EncryptionTestParam{true, true,
true});
// Base class to test writing and reading encrypted dataset.
-class DatasetEncryptionTestBase : public
testing::TestWithParam<EncryptionTestParam> {
+template <typename T, typename = typename std::enable_if<
+ std::is_base_of<EncryptionTestParam,
T>::value>::type>
Review Comment:
No need to change this, but as a nit it can probably be simplified to
```suggestion
template <typename T, typename = typename std::enable_if_t<
std::is_base_of_v<EncryptionTestParam, T>>
```
##########
python/pyarrow/tests/parquet/encryption.py:
##########
@@ -41,6 +41,8 @@ def wrap_key(self, key_bytes, master_key_identifier):
def unwrap_key(self, wrapped_key, master_key_identifier):
"""Not a secure cipher - just extract the key from
the wrapped key"""
+ if master_key_identifier not in self.master_keys_map:
+ raise ValueError("Unknown master key", master_key_identifier)
Review Comment:
Why not keep the `KeyError` that the line below would raise?
##########
python/pyarrow/tests/test_dataset_encryption.py:
##########
@@ -82,11 +88,11 @@ def create_decryption_config():
return pe.DecryptionConfiguration(cache_lifetime=300)
-def create_kms_connection_config():
+def create_kms_connection_config(keys):
return pe.KmsConnectionConfig(
custom_kms_conf={
- FOOTER_KEY_NAME: FOOTER_KEY.decode("UTF-8"),
- COL_KEY_NAME: COL_KEY.decode("UTF-8"),
+ key_name: key.decode("UTF-8") if isinstance(key, bytes) else key
Review Comment:
I'm not sure we want to allow both `bytes` and `str` key names 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
