First implementation for ExternalEncryptionConfiguration#15
Conversation
|
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: |
|
@sofia-tekdatum I found this code at the existing Encryptor as you can notice here is where column_keys changes to string format before get into the C++ code. I want to check if you still wan to use this pattern for the app_context |
Yes, we want that string passed as is all the way to the ExternalEncryptorImpl |
| internal_key_material=internal_key_material, | ||
| data_key_length_bits=data_key_length_bits) | ||
| #Holds a shared pointer to the underlying C++ external encryption configuration object. | ||
| self._external_config = shared_ptr[CExternalEncryptionConfiguration]( |
There was a problem hiding this comment.
Then here, we shouldn't be doing a new call at all, but rather a self.external_config.reset
There was a problem hiding this comment.
This is important to above errors during the destruction of the objects when the test ends. But I will see if the changes in the previous comment works there is a possibility to make this change
88ccd14 to
a8c6fa5
Compare
There was a problem hiding this comment.
nit: add space after comma
There was a problem hiding this comment.
Also print the value here as in the @Property exception above.
There was a problem hiding this comment.
Define separate test methods for each test case you want to create.
i.e. one for EncryptionConfiguration only
Another for ExternalEncryptionConfiguration with correct params
Another for ExternalEncryptionConfiguration with a poorly formed json
Don't just test the "sunny" cases.
a8c6fa5 to
0e69beb
Compare

Introducing the new ExternalEncryptionConfiguration request in #24