Fixing problems in ExternalEncryptionConfig bindings#66
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: |
avalerio-tkd
left a comment
There was a problem hiding this comment.
Thanks for the change. If it's helpful we can go over the change offline to understand it better.
|
|
||
| @column_keys.setter | ||
| def column_keys(self, dict value): | ||
| EncryptionConfiguration.column_keys.__set__(self, value) |
There was a problem hiding this comment.
To understand the pattern: Is this because it's the superclass that contains the dictionaries, not the subclass? And in the next line the subclass gets a copy? I can also ask you offline.
There was a problem hiding this comment.
The values are not propagated. The base fields are set in the base CEncryptionConfiguration when EncryptionConfiguration is called, but this unwrap_external returns the separate CExternalEncryptionConfiguration that never received those values, so C++ sees empty defaults.
There was a problem hiding this comment.
Discussed offline. I got the pattern now. Let's just add a comment about the superclass treatment of the value before assignment to the subclass.
There was a problem hiding this comment.
Added the comments and small fix on the crypto factory test
avalerio-tkd
left a comment
There was a problem hiding this comment.
Thanks for the fixes.
Fixing problems in the ExternalEncryptionConfig Cython bindings.
While debugging PR #57 I noticed the test setup was incorrect (repeated columns in per_column_encryption and column_keys). Since that should've caused exceptions, I started debugging whether the ExternalEncryptionConfig made its way correctly to the Arrow code.
Found and fixed the errors, plus a small bug in crypto_factory.cc related to the footer_key.
Fixed the bindings and the test.
Verified using the base app and sending an ExternalEncryptionConfig and printing out values in the CryptoFactory (there's no good way to test this right now).
Up next, fixing the rest of the existing bindings (ExternalDecryptionConfig and ExternalFileEncryptionProperties) until I make my way back to fixing the code in PR #57