Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for unpickleable datetime tzs set by snowflake #1670

Merged
merged 1 commit into from
Aug 13, 2019

Conversation

drewbanin
Copy link
Contributor

The snowflake-connector-python library does something... surprising... when handling timezones. It dynamically generates its own timezone objects which are not pickleable. As a result, simple queries like:

select current_timestamp::TIMESTAMP_TZ as oops

will fail if run on the rpc server. This happens specifically when data is passed between subprocesses over a queue.

Relevant issue: snowflakedb/snowflake-connector-python#81
Relevant code: converter.py#L140

I'm unsure if the approach I took here is a good one. I think I'd prefer for this code to live closer to the RPC server, but wasn't exactly sure if we had a good place to put this today. Very open to feedback / insight here.

fixed_row = []
for col in row:
if isinstance(col, datetime.datetime):
col = col.astimezone(tz=pytz.UTC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach a lot: force-converting everything to UTC seems fine to me and this is the right place to do it I think (I'd rather this than have RPC be special and funky, if we can avoid it, right?)

The only suggestion I have is that maybe the base class should do the [dict(zip(column_names, row)) for row in results] in its process_results, and snowflake should just do this and then return super(SnowflakeConnector, self).process_results(fixed) at the end? That's more of a style thing though and I don't have strong feelings about it, converting to a dict just feels like it's definitely "processing results".

@drewbanin drewbanin force-pushed the fix/snowflake-unpickleable-datetime-timezone branch from 909472f to fe3986b Compare August 9, 2019 18:54
@drewbanin drewbanin requested a review from cmcarthur August 9, 2019 20:53
@drewbanin
Copy link
Contributor Author

hey @cmcarthur - mind giving this one a look when you get a chance? I ended up porting the code from snowflakedb/snowflake-connector-python#188, as this will likely be a quicker route to getting this code into production.

return datetime.timedelta(0)

def __repr__(self):
return self.name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't define this class -- use pytz.FixedOffset instead

Copy link
Member

@cmcarthur cmcarthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@cmcarthur
Copy link
Member

code looks fine, but flake8 is broken

@drewbanin drewbanin force-pushed the fix/snowflake-unpickleable-datetime-timezone branch from e78a08d to 50fa1ba Compare August 12, 2019 17:47
@drewbanin drewbanin merged commit 2aee9ee into dev/0.14.1 Aug 13, 2019
@drewbanin drewbanin deleted the fix/snowflake-unpickleable-datetime-timezone branch August 13, 2019 17:17
@drewbanin drewbanin added this to the 0.14.1 milestone Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants