-
Notifications
You must be signed in to change notification settings - Fork 262
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
use execute_many from backends directly #468
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -278,7 +278,7 @@ async def execute_many( | |
) -> None: | ||
queries = [self._build_query(query, values_set) for values_set in values] | ||
async with self._query_lock: | ||
await self._connection.execute_many(queries) | ||
await self._connection.execute_many(queries, values) | ||
Comment on lines
279
to
+281
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we avoid building the queries here if you're going to compile them later? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, it's been a long time since I wrote this PR, but I think we still need to convert the query to ClauseElement and bind params, but maybe we don't need list comprehension here, so maybe something like that is sufficient
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm interesting. I'm not sure myself, just wanted to check if you had considered it. Thanks for chiming in on this after the delay. If you're interested in working on it again I can review and release it. Did you confirm this works and improves performance as expected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can work on it tmr for a few. |
||
|
||
async def iterate( | ||
self, query: typing.Union[ClauseElement, str], values: dict = None | ||
|
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 presume this it not a breaking change for users because this is an internal method and
values
is always provided by us in the public method?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'm not sure I understand what you mean here, but I think users are only supposed to use execute_many from the core file, not from any of the backends
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.
Yep that's the question — I want to gauge if changing this function signature is a breaking change.
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.
yea I don't think it should break anything