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 connection parameters, execute non-prepared statements #26

Closed
wants to merge 9 commits into from

Conversation

tdawber
Copy link

@tdawber tdawber commented Dec 28, 2016

This pull request enables using the AWS Athena JDBC driver, which requires certain properties passed at connection time, and does not support prepared statements.

Code for connection properties is appropriated from @Melraidin.

It will, by default, execute queries as standard statements, unless properties (including an empty parameter set) are passed into the execute function.

@coveralls
Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage decreased (-6.9%) to 72.406% when pulling 2cd93ef on tdawber:master into 9d71d12 on baztian:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-6.9%) to 72.406% when pulling 2cd93ef on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.9%) to 72.406% when pulling 2cd93ef on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.9%) to 72.406% when pulling 2cd93ef on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 4131806 on tdawber:master into 9d71d12 on baztian:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 4131806 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 4131806 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage decreased (-16.5%) to 62.736% when pulling 3542a80 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.5%) to 62.736% when pulling ae4acb6 on tdawber:master into 9d71d12 on baztian:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-16.5%) to 62.736% when pulling ae4acb6 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling e0f0762 on tdawber:master into 9d71d12 on baztian:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling e0f0762 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling e0f0762 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 6b25213 on tdawber:master into 9d71d12 on baztian:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 6b25213 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 6b25213 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 6b25213 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 6b25213 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 8f7849a on tdawber:master into 9d71d12 on baztian:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 78.302% when pulling 8f7849a on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage decreased (-0.5%) to 78.774% when pulling e2a1c10 on tdawber:master into 9d71d12 on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 78.774% when pulling e2a1c10 on tdawber:master into 9d71d12 on baztian:master.

@baztian
Copy link
Owner

baztian commented Jan 8, 2017

Thank you for your pull request.

Regarding the "non-prepared statements" aspect: Do I get it right, that if you don't pass any parameters the statement won't be prepared and otherwise it will be prepared? If so, how do you pass parameters to AWS Athena then?

@tdawber
Copy link
Author

tdawber commented Jan 9, 2017

Hey @baztian, that was my intention, yep. It's a pretty broad change, so not too sure how you'd feel about it. But, at the moment, it's not possible to use jaydebeapi with JDBC drivers that don't support prepared statements. Rather than create a separate 'execute'-type method for non-prepared statements, it seemed like a good technical fit to only use prepared statements when there were properties supplied?

The Athena JDBC driver doesn't support prepared statements; though maybe it will in the future. We've had issues with prepared statements in the AWS Hive JDBC driver too.

@tdawber
Copy link
Author

tdawber commented Jan 9, 2017

I know that coveralls is failing on this branch, but I'm not too sure where I could add further tests to increase coverage. New functionality is tested. But, the fact that I'm adding code to init.py decreases coverage overall.

@baztian
Copy link
Owner

baztian commented Jan 9, 2017

Regarding the connection parameters I'm working on a branch connection-properties to resolve the issue.
Regarding non prepared statements I would prefer another parameter to the connect method that defaults to false.

@baztian
Copy link
Owner

baztian commented Jan 10, 2017

Starting with 1.0.0 connection parameters are supported.

@tdawber
Copy link
Author

tdawber commented Jan 10, 2017

Thanks, @baztian! I'll review your changes and submit another pull request in the near future with your recommended implementation re prepared vs non-prepared statements.

@baztian
Copy link
Owner

baztian commented Jan 21, 2017

Closing as it is superseded by #29

@baztian baztian closed this Jan 21, 2017
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.

None yet

4 participants