Conversation
There was a problem hiding this comment.
FYI @yossariano -- I updated this again so that the name of the logger would be output. I also think this means that different struct loggers will now act a little more independently, which is good, although I could be wrong
|
|
||
| def __init__(self, account, user, private_key=None, password=None): | ||
| self.logger = create_log('snowflake_client') | ||
| if (password is None) == (private_key is None): |
There was a problem hiding this comment.
Woah I've never seen a boolean like this before, nice thinking!
| 3) This method can be used for both read and write queries, but | ||
| it's not optimized for writing -- there is no parameter binding | ||
| or executemany support, and the return value for write queries | ||
| can be unpredictable. |
There was a problem hiding this comment.
Oh this is annoying of Snowflake -- can you explain why you decided against using the executemany() function provided? Seems like that allows for parameterization (albeit clunkier)
There was a problem hiding this comment.
Yeah in the future we may add transaction support more similar to what we do in the Redshift client, and I can see executemany going in there. I held off putting it in here because: a) I didn't want to jam every possible functionality into the same execute_query function, and b) it's actually unclear to me whether we'll need to be writing to Snowflake that much using this client -- it seems like the more "data lake-y" way would be to upload new files to S3 instead.
Ultimately, I wanted a function that could read data and it just so happened to be that you can also execute arbitrary single SQL commands the same way, but the intention wasn't really to support that as a main use case.
|
|
||
| If kms_client is not empty, decrypts the variables first. | ||
|
|
||
| Does not allow for sub-dictionaries. |
There was a problem hiding this comment.
Is there any chance we'd use sub-dictionaries in our configs in the future? Or is that frowned upon?
There was a problem hiding this comment.
I'm not against it! I just think the use case for this file is loading config variables as environment variables, and it's unclear what the user would expect in the case of a subdictionary. Do all of the sub-keys get loaded as their own env variables, or does the whole dict get loaded as a JSON string, or? In general, my feeling is I haven't had a use case that required using sub-dictionaries, so there's no use trying to over-engineer this for a hypothetical use case.
fatimarahman
left a comment
fatimarahman
left a comment
There was a problem hiding this comment.
Nice! Just a couple of clarifying questions
|
Bypassing because approval was already given and only an extremely minor change was committed afterwards. I've changed the merge rules to prevent approvals from expiring in the future. |
2 participants
Also updated pytest (to silence #52) and GitHub Actions python versions.
See CHANGELOG for additional changes.