-
Notifications
You must be signed in to change notification settings - Fork 256
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
looks like i found a consistent way for sqlalchemy to implement this: will rework the PR and add method |
aiomysql/sa/connection.py
Outdated
if not compiled: | ||
compiled = query.compile(dialect=self._dialect) | ||
if ( | ||
dp and dp.keys() == compiled.params.keys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the formatting looks weird
Sorry for late reply, after first look PR looks good, I will review more carefully this weekend. |
aiomysql/sa/connection.py
Outdated
compiled = query.compile(dialect=self._dialect) | ||
# parameters = compiled.params | ||
if self._compiled_cache is not None: | ||
key = (self._dialect, query) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Thanks! |
new version available https://pypi.org/project/aiomysql/0.0.16/ |
Ability to execute pre-compiled queries removes ~30% sqlalchemy compilation overhead when the query is run repeatedly.