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

Support custom requests session in presto connections #106

Merged

Conversation

gthomas-slack
Copy link
Contributor

pyhive supports passing through a custom requests session for advanced usage such as custom headers, cookie values, retry logic, etc - https://github.com/dropbox/PyHive/blob/master/pyhive/presto.py#L115

This PR adds support for passing through a custom requests session to pyhive when creating an omniduct PrestoClient.

"""
catalog (str): The default catalog to use in database queries.
schema (str): The default schema/database to use in database queries.
server_protocol (str): The protocol over which to connect to the Presto REST
service ('http' or 'https'). (default='http')
source (str): The source of this query (by default "omniduct <version>").
If manually specified, result will be: "<source> / omniduct <version>".
requests_session (requests.Session): an optional ``requests.Session`` object for advanced usage.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: single backticks here on requests.Session

"""
self.catalog = catalog
self.schema = schema
self.server_protocol = server_protocol
self.source = source
self.__presto = None
self.connection_fields += ('catalog', 'schema')
self.requests_session = requests_session
Copy link
Collaborator

Choose a reason for hiding this comment

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

likely we should have the be private (ie. start with an underscore)

Copy link
Collaborator

@danfrankj danfrankj left a comment

Choose a reason for hiding this comment

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

Hey @gthomas-slack - thanks for this and for contributing to omniduct! A couple nits but nothing significant. I'll let @matthewwardrop comment as well.

@gthomas-slack
Copy link
Contributor Author

Hey @gthomas-slack - thanks for this and for contributing to omniduct! A couple nits but nothing significant. I'll let @matthewwardrop comment as well.

Thanks @danfrankj ! I've pushed a commit to address those nits

@matthewwardrop
Copy link
Collaborator

Hi @gthomas-slack ! Thanks for the patch! Merging it in now! [I'll put out a new release soon].

FYI: Eventually, I had planned to migrate omniduct from using pyhive to the upstream presto-python-client. It's blocked on them adding support for polling, for which I have a PR open (trinodb/trino-python-client#18); but which I've let lie fallow for a while since the world has gone crazy and I don't have a huge amount of bandwidth (and the review cycles on that PR are very slow). For the time being, it looks like pyhive is still being maintained, so there's no harm in continuing to bolster features. If the libraries are not at parity when this migration occurs, assuming it does, then I'll keep the backends switchable (so this patch shouldn't break) :).

@matthewwardrop matthewwardrop merged commit c067777 into airbnb:master Oct 9, 2020
@gthomas-slack
Copy link
Contributor Author

Thanks @matthewwardrop !

Ah interesting - if/when it comes time to fully switchover to presto-python-client I'd be happy to help look into what changes we might need on that side to maintain this support. Thanks again

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