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

ability to execute precompiled sqlalchemy queries #294

Merged
merged 4 commits into from
Jun 3, 2018
Merged

ability to execute precompiled sqlalchemy queries #294

merged 4 commits into from
Jun 3, 2018

Conversation

vlanse
Copy link
Contributor

@vlanse vlanse commented May 21, 2018

Ability to execute pre-compiled queries removes ~30% sqlalchemy compilation overhead when the query is run repeatedly.

@codecov
Copy link

codecov bot commented May 21, 2018

Codecov Report

Merging #294 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
+ Coverage   92.79%   92.85%   +0.05%     
==========================================
  Files           9        9              
  Lines        1110     1119       +9     
  Branches      158      161       +3     
==========================================
+ Hits         1030     1039       +9     
  Misses         56       56              
  Partials       24       24
Impacted Files Coverage Δ
aiomysql/sa/connection.py 92.57% <100%> (+0.35%) ⬆️
aiomysql/sa/engine.py 88.63% <100%> (+0.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6f3ee9...784432f. Read the comment docs.

@vlanse
Copy link
Contributor Author

vlanse commented May 21, 2018

looks like i found a consistent way for sqlalchemy to implement this:

http://docs.sqlalchemy.org/en/latest/core/connections.html#sqlalchemy.engine.Connection.execution_options.params.compiled_cache

will rework the PR and add method execution_options supporting compiled_cache parameter to sa connection class

if not compiled:
compiled = query.compile(dialect=self._dialect)
if (
dp and dp.keys() == compiled.params.keys()
Copy link
Member

Choose a reason for hiding this comment

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

the formatting looks weird

@vlanse
Copy link
Contributor Author

vlanse commented May 28, 2018

@asvetlov @jettify
can you please proceed with review?

@vlanse
Copy link
Contributor Author

vlanse commented Jun 1, 2018

@asvetlov @jettify
any suggestions here? the problem hurts, so it would be great if the fix will be merged

@jettify
Copy link
Member

jettify commented Jun 1, 2018

Sorry for late reply, after first look PR looks good, I will review more carefully this weekend.

compiled = query.compile(dialect=self._dialect)
# parameters = compiled.params
if self._compiled_cache is not None:
key = (self._dialect, query)
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need self. _dialect as a part of the key, since it is always the same object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, a kind of overkill)


def test_cache(self):
async def go():
cache = dict()
Copy link
Member

Choose a reason for hiding this comment

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

Pure dict is not option for production system, since it has unbounded size. Should we also provide basic cache implementation also? something like LRU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is up to implementor. LRU could be sufficient in most cases
BTW, sqlalchemy also does not provide any defaults - its only demand is that cache is some dict-like object

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense

@jettify
Copy link
Member

jettify commented Jun 3, 2018

Thanks!

@jettify jettify merged commit aec31dd into aio-libs:master Jun 3, 2018
@jettify
Copy link
Member

jettify commented Jun 3, 2018

new version available https://pypi.org/project/aiomysql/0.0.16/

@Nothing4You Nothing4You mentioned this pull request Jun 11, 2023
4 tasks
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