Adding wiring for ExternalEncryption and ExternalDecryption objects i… - #255
Conversation
…n ParquetEncryptionConfig, ParquetDecryptionConfig, ParquetFragmentScanOptions, and ParquetFileWriteOptions
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thank for you this. LGTM - but left a couple of comments (main one around testing coverage). No need for a new PR if/when addressed.
| }; | ||
|
|
||
| struct ExternalConfigTestParam { | ||
| bool uniform_encryption; // false is using per-column keys |
There was a problem hiding this comment.
why not call this bool use_per_column_encryption?
There was a problem hiding this comment.
Removed this.
|
|
||
| struct ExternalConfigTestParam { | ||
| bool uniform_encryption; // false is using per-column keys | ||
| bool concurrently; |
There was a problem hiding this comment.
what's this for? (let's add a comment)
There was a problem hiding this comment.
Restructured the test and removed this.
| } | ||
| }; | ||
|
|
||
| struct ExternalConfigTestParam { |
There was a problem hiding this comment.
High level comment for this file - there is a lot going on, but mostly it seems that we're just testing that the configuration(s) passed in are correct. Maybe I missed something, but I don't think I got to see a test where we actually exercise external encryption (e.g. external encrypt/decrypt roundtrip).
There was a problem hiding this comment.
It was mostly a plumbing test only. Added explicit testing of the external encryption in a column and aes encryption in another column.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thank you for the changes. LGTM. Ship it!
2 participants
Add wiring for ExternalEncryptionConfig and ExternalDecryptionConfig in ParquetEncryptionConfig and ParquetDecryptionConfig.
Also add support for ExternalFileDecryptionProperties in ParquetFragmentScanOptions.
These were entry points that were missing. I have checked and after any of these are created in C++, the code creates FileReader and FileWriter objects, which already know how to deal with External encryption and decryption cases.
Adding tests to cover the new cases.