-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(python): Add support for async
SQLAlchemy connections to read_database
#15162
feat(python): Add support for async
SQLAlchemy connections to read_database
#15162
Conversation
Looks like some slight differences in the latest async behaviour; the new unit tests pass on Python |
107e159
to
8c4fb0c
Compare
8c4fb0c
to
0502c8d
Compare
Got it; all tests passing on all platforms now 😎 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15162 +/- ##
=======================================
Coverage 81.18% 81.18%
=======================================
Files 1347 1347
Lines 175423 175467 +44
Branches 2506 2516 +10
=======================================
+ Hits 142410 142449 +39
- Misses 32533 32536 +3
- Partials 480 482 +2 ☔ View full report in Codecov by Sentry. |
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.
Looks good, thanks!
Just one comment as an aside: I usually extract private functions below the main function, following the "newspaper style" proposed by uncle Bob. I notice you usually go the other way (there's also something to be said for that).
I often find myself moving functions around when refactoring, which is not ideal for getting clean git diffs/history. Are you very attached to your current modus operandi?
I guess I think the other way round - if you're going to use something, it should already have been defined, so you will know it's available and have seen it coming (rather than using a newspaper analogy I find it more like a house; you don't build a house from the top-floor down, you build it from the foundations up 🤣) However, I'm about to make that difference largely go away here as my follow-up PR (already ready to go!) refactors the database code into a clean |
Sounds good! |
Here we go: #15201 |
@alexander-beedie I just got a spurious failure in the CI, I think it might be related to the async test added in this PR as it looks async-ish. What do you think? |
Will have a look in a few minutes! |
after installing "nest_asyncio" i finally got the engine to work with it. but due to reasons of setting variables and schema etc first i need to use the "session" instead. i cant get it working with the session. pls sir.. can i have some help - puppy dog eyes - |
Ref: https://stackoverflow.com/questions/77166897
Database connectivity continues to expand; with this PR we can now seamlessly init a
DataFrame
from async SQLAlchemyConnection
,Engine
, andSession
objects.Added async unit test coverage using
aiosqlite
(SQLite), but additionally validated the PR locally usingasyncpg
(PostgreSQL) andaioodbc
(ODBC) drivers.Examples
SQL Server over ODBC (async driver):
SQLite (async driver):
Also
The "execute" method of the internal ConnectionExecutor abstraction was getting a bit long, so factored out the SQLAlchemy setup block into its own method for clarity. Next commit will likely be a larger-scale (and overdue ;) refactor of all the database code into its own "io.database" subdirectory to facilitate easier maintenance.