Skip to content

Support connection parameters, execute non-prepared statements#26

Closed
tdawber wants to merge 9 commits into
baztian:masterfrom
tdawber:master
Closed

Support connection parameters, execute non-prepared statements#26
tdawber wants to merge 9 commits into
baztian:masterfrom
tdawber:master

Conversation

@tdawber

@tdawber tdawber commented Dec 28, 2016

Copy link
Copy Markdown

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

coveralls commented Dec 28, 2016

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Dec 28, 2016

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Dec 28, 2016

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Dec 28, 2016

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Dec 28, 2016

Copy link
Copy Markdown

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
Copy Markdown

Coverage Status

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

@coveralls

coveralls commented Dec 28, 2016

Copy link
Copy Markdown

Coverage Status

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@baztian

baztian commented Jan 8, 2017

Copy link
Copy Markdown
Owner

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

tdawber commented Jan 9, 2017

Copy link
Copy Markdown
Author

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

tdawber commented Jan 9, 2017

Copy link
Copy Markdown
Author

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

baztian commented Jan 9, 2017

Copy link
Copy Markdown
Owner

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

baztian commented Jan 10, 2017

Copy link
Copy Markdown
Owner

Starting with 1.0.0 connection parameters are supported.

@tdawber

tdawber commented Jan 10, 2017

Copy link
Copy Markdown
Author

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

baztian commented Jan 21, 2017

Copy link
Copy Markdown
Owner

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.

4 participants