Change connection_config from a string:string map to a map from Parqu…#56
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 don't understand this PR. Let's chat offline quickly to get more context. |
avalerio-tkd
left a comment
There was a problem hiding this comment.
This is just the change in the connection_config to add the algorithm_enum as key. Discussed offline. Thanks.
7ccd112 to
4d4c72f
Compare
|
@avalerio-tkd After discussin with @cristekdatum this afternoon, I added the changes to the Python bindings as well. Can you PTAL? |
avalerio-tkd
left a comment
There was a problem hiding this comment.
LGTM+. I had a couple of questions, but not blocking to merge. Thanks.
There was a problem hiding this comment.
Why is this clear() needed? Are we reusing an auxiliary data struct? Let's add a comment.
There was a problem hiding this comment.
Yes, clearing from the values of the previous iteration. Comment added.
There was a problem hiding this comment.
Any chance of these look ups failing? Seems to follow the pattern of the previous version, but just checking.
There was a problem hiding this comment.
In this specific line no lookup is happening, but in general, no. If there is an incorrect value sent in as ParquetCipher the cipher_to_name and cipher_from_name methods will catch this and raise ValueErrors accordingly.
…etCipher to another map of string:string
304c1a7 to
48f025b
Compare
Change the connection config from a map to a map<ParquetCipher : map>
This will allow us the flexibility of specifying a connection config for more than one external encryptor at a time.
Also changed in Cython bindings.