-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Enable server side cursors #46166
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
Enable server side cursors #46166
Conversation
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.
is this exercised in any current tests?
does the doc-string need updating here?
def execute(self, *args, **kwargs): | ||
"""Simple passthrough to SQLAlchemy connectable""" | ||
return self.connectable.execution_options().execute(*args, **kwargs) | ||
def execute(self, chunksize: int = 0, *args, **kwargs): |
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.
can you add a return type
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.
Will do! Will also add references to relevant tests and add a test if needed
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
Hello @J0! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge the main branch, address the review comments, and we can reopen. |
@J0 Is this still being worked on? |
The old PR went stale and I ended up busy for a year so had to re-open in a new branch. Hoping to finally push this through
This pull request attempts to fix #35689. I read an article by @itamarst and decided to look further into the codebase. After noticing that the fix semed to be simple I decided file a PR. The fix is naive and I'm not sure the true solution is as simple as I imagine it to be. There are probably many things I'm missing as I've not read the codebase in detail so do let me know what else needs to be addressed.