-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix(python): Fixes a read_database
issue loading specific datetime types from SQL Server backends
#14627
fix(python): Fixes a read_database
issue loading specific datetime types from SQL Server backends
#14627
Conversation
…types from SQL Server 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.
Looks good to me - feel free to merge if CI is green.
If it's possible to test (part of) this locally, adding a test would be good.
Hmm, I might be able to mock something, but what we really need are some ephemeral DBs integrated into the CI pipeline1 so we can do a better job covering (PostgreSQL and SQL Server have official Docker images; no doubt a few others do too). Footnotes |
I didn't really intend for you to set up a complicated mocking structure for this PR 😄 I was just thinking maybe extract part of the logic as a testable function. But it's up to you! |
Tricky... it needs a pretty specific interaction with the DB cursor to expose it; without mocking it would be hard to tease it out from inside I did find a related gremlin that's much easier to isolate though; will create a separate Issue for that shortly. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #14627 +/- ##
==========================================
- Coverage 80.84% 80.83% -0.01%
==========================================
Files 1326 1326
Lines 173065 173071 +6
Branches 2450 2453 +3
==========================================
- Hits 139907 139903 -4
- Misses 32684 32693 +9
- Partials 474 475 +1 ☔ View full report in Codecov by Sentry. |
Closes #11912.
Very slightly reworks cursor introspection/load for
read_database
, avoiding a SQL Server issue withDATETIMEOFFSET
cols. Validated against a local SQL Server Docker image with a suitable test table...(Also improves ODBC connection string detection as a trivial drive-by).